Skip to content
This repository was archived by the owner on May 12, 2026. It is now read-only.

Move note between folders#374

Merged
asmsuechan merged 9 commits into
masterfrom
feature_move_note_between_folder
May 20, 2017
Merged

Move note between folders#374
asmsuechan merged 9 commits into
masterfrom
feature_move_note_between_folder

Conversation

@sosukesuzuki

Copy link
Copy Markdown
Contributor

I made it allows move note between folders by drag and drop.
move_note_between_folders 1

Comment thread browser/main/SideNav/StorageItem.js Outdated
type: noteData.type,
updatedAt: noteData.updatedAt,
description: noteData.description,
snippets: noteData.snippets

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Object.assign() is better in this situation. const newNoteData = Object.assign({}, noteData, { storage: storage, folder: folder.key })

Comment thread browser/main/SideNav/StorageItem.js Outdated
e.target.style.opacity = '1'
e.target.style.backgroundColor = e.dataTransfer.getData('defaultColor')
const noteData = JSON.parse(e.dataTransfer.getData('note'))
if (folder.key !== noteData.folder) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce nest, I prefer to use if (folder.key === noteData.folder) return

Comment thread browser/main/SideNav/StorageItem.js Outdated
import RenameFolderModal from 'browser/main/modals/RenameFolderModal'
import dataApi from 'browser/main/lib/dataApi'
import StorageItemChild from 'browser/components/StorageItem'
import ee from 'browser/main/lib/eventEmitter'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change ee to eventEmitter? We're trying to use the name in full.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for commenting. I'll change it.

Comment thread browser/main/SideNav/StorageItem.js Outdated
query: {key: note.storage + '-' + note.key}
})
})

@asmsuechan asmsuechan Apr 7, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to handle rollback this copy process just in case it does not work. In this state, only to be copied the note without delete if something happens in copying.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks commenting @asmsuechan .I agree with your opinion. So, I changed to Deleting the folder only when the folder creation succeeds.


handleDragLeave (e) {
e.target.style.opacity = '1'
e.target.style.backgroundColor = e.dataTransfer.getData('defaultColor')

@asmsuechan asmsuechan Apr 7, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change the background-color? Because it's hard to find the target folder.

@asmsuechan asmsuechan May 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sosukesuzuki what do you think about this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for commenting @asmsuechan !
Even now, the background color of the folder that is hover while dragging is supposed to change. How do you want the background color to change?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't confirm the color changed.

@jasondavis

Copy link
Copy Markdown

I was about to post this as a feature request as many people need this feature too. So it's nice to see you already did it here!

Comment thread browser/main/SideNav/StorageItem.js Outdated
.then((note) => {
dataApi
.deleteNote(noteData.storage, noteData.key)
.then((data) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unnecessary space 😎

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for commenting ! I'll fix it soon.

Comment thread browser/main/SideNav/StorageItem.js Outdated
})
hashHistory.push({
pathname: location.pathname,
query: {key: note.storage + '-' + note.key}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use template literals like ${note.storage}-${note.key}.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for commenting. I'll fix it soon.
However, such a code is often used in Boostnote. For example, there are many other codes that use for, not forEach. We should fix it. I'll do it before long.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please!

@asmsuechan asmsuechan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnamed

@asmsuechan asmsuechan merged commit 3c7ff78 into master May 20, 2017
@asmsuechan asmsuechan deleted the feature_move_note_between_folder branch May 20, 2017 05:33
@kohei-takata kohei-takata mentioned this pull request Jun 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

4 participants