-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
- Permanently deleting note would not remove note from list until after refresh - Fix so permanently deleted note is removed list
I see, I will take a look! |
3fbed87
to
dc03bb7
Compare
@@ -200,6 +200,7 @@ class MarkdownNoteDetail extends React.Component { | |||
} | |||
ee.once('list:moved', dispatchHandler) | |||
}) | |||
.then(() => ee.emit('list:moved')) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
Updated to use |
Test failure seems like a flaky test (also seen happening here on master). Reran locally and passed. |
There was a problem hiding this 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!
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.