Skip to content

Conversation

@ehhc
Copy link
Contributor

@ehhc ehhc commented May 15, 2018

@Rokt33r we had a slack discussion that it is neccessary to change the note content since the noteKey is changed during the move and the attachments should be in the correct folder.
We discussed whether it is possible to simply remove this new-creation of the note key. I tried it but using the old noteKey leads to some strange issues when the app tries show the moved note. So i guess that's the reason why you initially included the new-key-feature. -> it can't be removed.

We agreed that it's a good idea to have the attachements in a folder matching the noteKey. So this PR is again needed for moving a note with its attachment.

You wished for a PR that you can auto-merge.
Here it is.

@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label May 16, 2018
Copy link
Member

@Rokt33r Rokt33r left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

const path = require('path')
const findStorage = require('browser/lib/findStorage')
const mdurl = require('mdurl')
const fse = require('fs-extra')
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need fs-extra? I assume sander can do the same job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used fs-extra because it has a good move-directory function. As far as i know sanders is not able to do so.. I think it contains some fallback implementation if the quick/efficient ways don't work..

@Rokt33r Rokt33r merged commit f717ed9 into BoostIO:master May 16, 2018
@Rokt33r Rokt33r added next release (v0.11.5) and removed awaiting review ❇️ Pull request is awaiting a review. labels May 16, 2018
@ehhc ehhc deleted the Moving_Note_With_Attachment branch May 16, 2018 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants