-
Notifications
You must be signed in to change notification settings - Fork 65
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
Bump react-router-dom from 6.2.1 to 6.4.2 in /frontend #4508
Bump react-router-dom from 6.2.1 to 6.4.2 in /frontend #4508
Conversation
Bumps [react-router-dom](https://github.com/remix-run/react-router/tree/HEAD/packages/react-router-dom) from 6.2.1 to 6.4.2. - [Release notes](https://github.com/remix-run/react-router/releases) - [Changelog](https://github.com/remix-run/react-router/blob/main/packages/react-router-dom/CHANGELOG.md) - [Commits](https://github.com/remix-run/react-router/commits/[email protected]/packages/react-router-dom) --- updated-dependencies: - dependency-name: react-router-dom dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
This is currently not working because in Prompt we use the navigate block from the library and that is not intended for use. In this last version (6.4.x) they decided to remove that functionality entirely. They have decided not to add it for now.
|
Thank you for your research and digging into this, Boban! While testing locally I did see the problem you described. In regards to your options: I was checking when and why we updated to v6 in the first place and it looks like it was to get to React 18: #3261 but based on the comment you linked Boban it looks like they support v5 for React 18 remix-run/react-router#8139 (comment) 🤔 |
It seems that we're using Prompt to block user navigation away from the PersonForm, FacilityForm, and UserDetail forms when they have unsaved data, which the maintainers call out specifically as a somewhat jarring UX pattern with a suggested replacement. Worth chatting with design about it, but I think their argument makes sense from a UX improvement perspective. The smoothest interaction in those forms would be just to persist / reload the unsaved data users input in the event they navigate away from an unsaved form. I could be wrong since I haven't interacted with those browser API's in an SR context before, but storing unsaved formed data in local/sessionStorage just in these three forms shouldn't that be that big of a technical lift? Should be as simple as storing the form input as a json against the url as a key and then reloading if it exists on renavigation to the page. cc: @kenieh Another consideration is whether we have any security issues persisting unsaved form data in session storage. It's technically PII, but since we're never persisting it in any data stores / it's only relevant in cases of unsaved form data, I'd hazard it's ok to handle it this way, but might be wrong here. cc: @rin-skylight Assuming that's true, if we're getting the UX benefits on top of moving away from a dependency, that's worth it in my mind. Worth bringing up to the rest of the team, but I'd go with Boban's third suggestion after digging a little. cc: @emmastephenson |
I think for a version bump PR, we shouldn't introduce any behavior changes. It is worth design considering if we should change the user experience, however I think it's out of scope for this kind of PR. I think we should either 1) implement the suggested workaround for now keeping the existing behavior and have design decide if/when to prioritize a change in the future or 2) decide not to bump the version right now and close this PR, and again design can decide if/when they want to make this change down the road. cc: @emmastephenson |
+1 to Elisa's comment about not wanting to downgrade to React Router v5 - it was a pretty big pain to upgrade in the first place as I recall, doesn't seem worth undoing just for this Prompt functionality. I definitely lean towards the suggested solution of moving away from Prompt entirely, but agree with @georgehager that it's out of scope for this PR. I created a new ticket for design to address (#4534), and in the meantime I think we should just close this PR - implementing the workaround doesn't seem like a good use of dev time if we decide to turn around and change the design. |
OK, I won't notify you again about this release, but will get in touch when a new version is available. If you'd rather skip all updates until the next major or minor version, let me know by commenting If you change your mind, just re-open this PR and I'll resolve any conflicts on it. |
Is this something we could discuss at our next G&E sync? I'm not sure I'm following this thread |
Sounds like there isn't an immediate need for a design solution? |
But yeah, no urgent need until then! |
Bumps react-router-dom from 6.2.1 to 6.4.2.
Release notes
Sourced from react-router-dom's releases.
... (truncated)
Changelog
Sourced from react-router-dom's changelog.
... (truncated)
Commits
92425b8
chore: Update version for release1317087
chore: Update version for release (pre) (#9395)5905109
chore: Update version for release (pre) (#9380)540f94b
fix: Strengthen route typings (#9366)434003d
fix: respect basename in useFormAction (#9352)779d4af
fix: update matchPath to avoid false positives on dash-separated segments (#9...d8e6d7f
docs: add better docs and console errors for data router features (#9311)d405320
chore: update versions for releasec4a27f7
chore: Update version for release (pre) (#9316)aeceb7d
fix changeset config + update changelogsDependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)