-
Notifications
You must be signed in to change notification settings - Fork 980
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
feat(router): add router useBlocker hook #10873
Conversation
Changed to draft until discussed. also may need some tests and docs. |
Thanks for picking this back up again @xmaxcooking 🙏 I'll have to read back up on our previous discussions before I can provide much meaningful input here. I'll try to get to it this week, please ping me if I don't |
I haven't forgotten about his PR, but I have some tight deadlines coming up (one for Monday, one for Tuesday and finally one for Wednesday). I've set a reminder to myself to check back on this PR on Thursday |
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 for the PR. Overall I like this :) Just a few things:
- Please fix all the errors and warnings
- Please add tests
- Take a look at my two comments and see what you think about them
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Thanks for the review! I'm going to polish this next week and then change it from draft to ready. |
Awesome @xmaxcooking! I’m taking today off, but will look when I get a chance |
…king/redwood into xmax-router-blocker-hook
Added the playwright test and removed the link changes👍 |
Thank you so much for the work you put into this feature @xmaxcooking! Very much not taken for granted 🙏 |
**Problem** The rebuild-test-project-fixture script was broken locally - and probably on CI too. **Changes** 1. Move generation of user sdl ahead of dbauth setup 2. Updates general version bumps in fixure 3. Updates for fragment type fix (on or around #10825 4. Updates for useBlocker in contact us form #10873 5. Updates to remove unused imports and have lint check complete successfully
Successor of: #9843
Can be used like this:
Tab close
,Browser Close
andNavigation
with the redwood router components and functions can be blocked.The only thing not working is the interception of the browser
back
button.I found possible solutions to block the navigation
back
too, but not without losing any user changes and/or messing with the history.React-router defined the situation like this: https://github.com/remix-run/react-router/blob/aacc2b94088835d1247bdf3a3f883f74cc3570a0/decisions/0001-use-blocker.md?plain=1#L37C1-L37C127
Blocking POP navigations is different - since we don't know about them via popstate until _after_ the URL has been updated
Currently the redwood router and the active page are coupled to the real location (URI) and it would require a refactoring to couple it with something like a location proxy instead which could be kept in sync with the history and location.
Any thoughts? Would a refactoring be worth it? Or is this implementation with a missing piece of functionality good enough?