-
Notifications
You must be signed in to change notification settings - Fork 7
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] allow users to use the browser-back button #650
Conversation
✅ Deploy Preview for neurobagel-annotator ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Nicely done @surchs! Handling of routes is elegant and easy to understand.
Aside from the comments, I have two 🍒
- I think we can remove
getNextPage
andsetCurrentPage
from the store. - I'd suggest adding a test for using the browser back button to one of the e2e tests.
Also remove some unnecessary cypress comands
The e2e test for |
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.
I think you forgot to remove getNextPage
and setCurrentPage
from the store and add a test for the browser back button.
No longer needed
@rmanaem can you confirm where you still see |
annotation_tool/store/index.js Lines 497 to 518 in 86c0abf
annotation_tool/store/index.js Lines 839 to 842 in 86c0abf
|
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.
Thanks @surchs! 🧑🍳
Changes proposed in this pull request:
A lot of the stuff in the store around route handling and pages is already taken care of by the router. This PR essentially removes all of this stuff and replaces it with functions from the router
One big issue with the old system was that it only looked to the store to find out what page it was currently on. so if I use the browser back button this is not reflected in the store, but the route changes. So I could end up e.g. on the home page with a state that thinks I'm on the annotation page.
So we can make this both easier and have it not break anymore
Checklist
[ENH]
,[FIX]
,[REF]
,[TST]
,[CI]
,[MNT]
,[INF]
,[MODEL]
,[DOC]
) (see https://neurobagel.org/contributing/pull_requests for more info)Closes #XXXX
For new features:
For bug fixes: