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: Wrap route with location arg in location context #9094

Conversation

johnpangalos
Copy link
Contributor

@johnpangalos johnpangalos commented Jul 25, 2022

In order for the useLocation hook to work with the use of the modal pattern, routes with the locationArg prop are wrapped in a LocationContext. This is done conditionally in order to ensure performance in the default case doesn't change.

closes #8470

@changeset-bot
Copy link

changeset-bot bot commented Jul 25, 2022

🦋 Changeset detected

Latest commit: 29f273b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@johnpangalos johnpangalos force-pushed the johnpangalos/fix-use-location-for-background-routes branch from 371b5bf to 5ed2b73 Compare Jul 25, 2022
@johnpangalos
Copy link
Contributor Author

johnpangalos commented Jul 25, 2022

Not sure how changesets are meant to work, they aren't mentioned in contributing document but the changeset bot has kindly reminded me about them.

@johnpangalos johnpangalos force-pushed the johnpangalos/fix-use-location-for-background-routes branch from 5ed2b73 to 9294f04 Compare Jul 25, 2022
@johnpangalos
Copy link
Contributor Author

johnpangalos commented Jul 25, 2022

I also bumped the file sizes half a kB as it seems that the code I added is now breaking that limit. Is there any guidance as what should be done when reaching that limit?

@johnpangalos johnpangalos force-pushed the johnpangalos/fix-use-location-for-background-routes branch 2 times, most recently from ee297e3 to fe1b0fb Compare Jul 25, 2022
@timdorr timdorr requested a review from brophdawg11 Jul 25, 2022
@timdorr
Copy link
Collaborator

timdorr commented Jul 25, 2022

For the changesets, just follow the links in the comment above. This change should result in a patch version bump (it fixes a bug), so I would add one.

As for the size thing, that's fine to bump up just a little. It's mainly a protection against adding something stupidly huge in a PR without anyone noticing.

@johnpangalos johnpangalos changed the title Wrap route with location arg in location context fix: Wrap route with location arg in location context Jul 26, 2022
@johnpangalos johnpangalos force-pushed the johnpangalos/fix-use-location-for-background-routes branch from fe1b0fb to 914ce9b Compare Jul 26, 2022
@brophdawg11
Copy link
Contributor

brophdawg11 commented Jul 26, 2022

Thanks for the PR @johnpangalos! I left a comment over on #8470 but I'll ping Michael and Ryan on this as well to get their take

@johnpangalos johnpangalos force-pushed the johnpangalos/fix-use-location-for-background-routes branch 2 times, most recently from 914ce9b to d0c38e2 Compare Aug 4, 2022
In order for the `useLocation` hook to work with the use of the modal
pattern, routes with the locationArg prop are wrapped in a
LocationContext. This is done conditionally in order to ensure
performance in the default case doesn't change.
@johnpangalos johnpangalos force-pushed the johnpangalos/fix-use-location-for-background-routes branch from d0c38e2 to 5dcc570 Compare Aug 5, 2022
@flying-sheep
Copy link

flying-sheep commented Sep 6, 2022

@mjackson, @ryanflorence, could you please take a look? I think a lot of people are eagerly waiting for this fix!

@brophdawg11
Copy link
Contributor

brophdawg11 commented Sep 12, 2022

Thanks @johnpangalos! Made a few small updates, but this looking good and I'll merge shortly

@brophdawg11 brophdawg11 merged commit e766ab5 into remix-run:dev Sep 12, 2022
2 checks passed
@johnpangalos
Copy link
Contributor Author

johnpangalos commented Sep 14, 2022

Sounds good, glad to see this merged! Thanks @brophdawg11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: useLocation inside <Routes location={location}> doesn't return given location
4 participants