Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix permanently deleting note #1483

Merged
merged 3 commits into from
Feb 10, 2018
Merged

Conversation

andyklimczak
Copy link
Contributor

This bug was created in 9f9463f (git bisect), and I'm not sure of what review comments the commit message is referring to (looked through a few PRs around that time). So this fix might not be the fix we really want, depending on those comments.

- Permanently deleting note would not remove note from list until after
refresh
- Fix so permanently deleted note is removed list
@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label Jan 30, 2018
@sosukesuzuki
Copy link
Member

thanks for your contributing.
Sorry, the code causing this bug was written by my review.
The reason why I did such a review is that because if we select cancel, Boostnote select next note, like below.
1 -30-2018 18-23-49
(This video was captured in this PR.)
If you can, I want you to fix it too.

@kazup01 kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Jan 30, 2018
@andyklimczak
Copy link
Contributor Author

I see, I will take a look!

@@ -200,6 +200,7 @@ class MarkdownNoteDetail extends React.Component {
}
ee.once('list:moved', dispatchHandler)
})
.then(() => ee.emit('list:moved'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue really is that dispatchHandler is never called. So we emit the event for the dispatch handler to be run.

I think this is better than just directly calling the dispatch handler (dispatchHandler()) because it remains asynch using the event emitter (I believe).

Copy link
Member

Choose a reason for hiding this comment

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

I see. I think it is better than calling the dispatch handler.
but, if we delete a note in trash, Boostnote select nothing, like below.
1 -31-2018 18-18-06
this behavior is not we hope.

@andyklimczak
Copy link
Contributor Author

Updated and included small explanation. I'm not 100% sure if this is the best way, there are a few ways to solve this (by calling the dispatch handler) that seem to have subtle differences.

Thanks!

@andyklimczak
Copy link
Contributor Author

Updated to use list:next so that the next note in the trash is selected (if one exists)

@andyklimczak
Copy link
Contributor Author

andyklimczak commented Feb 1, 2018

Test failure seems like a flaky test (also seen happening here on master).

Reran locally and passed.

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

Sorry, our CI is broken...

Thank you for your fix.:smile:
I'll approve:+1:
LGTM!

@Rokt33r Rokt33r added next release (v0.9.0) and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Feb 6, 2018
@Rokt33r Rokt33r merged commit 7a124c7 into BoostIO:master Feb 10, 2018
@andyklimczak andyklimczak deleted the delete-note branch February 10, 2018 21:58
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.

Deletion of a note in Trash isn't effective until reload
4 participants