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

Add location prop to useRoutes() for e.g. animated transitions between routes #7298

Closed
wants to merge 6 commits into from

Conversation

MeiKatz
Copy link
Contributor

@MeiKatz MeiKatz commented Apr 30, 2020

Maybe someone can tell me how I can create a branch from a commit without copying all the commits from the contributors before it. Or this okay this way?

@timdorr timdorr changed the base branch from master to dev April 30, 2020 18:51
@timdorr
Copy link
Member

timdorr commented Apr 30, 2020

You were based on the master branch. I switched it to dev.

@MeiKatz
Copy link
Contributor Author

MeiKatz commented Apr 30, 2020

Thank you!

@laurencefass
Copy link

ive subscribed to this thread. will it get updated if/when merged to the main branch? thanks.

@MeiKatz
Copy link
Contributor Author

MeiKatz commented May 12, 2020

@laurencefass I will add a comment when my pull request got merged.

@gustavopch
Copy link

Will this get merged?

@MeiKatz
Copy link
Contributor Author

MeiKatz commented Jun 15, 2020

@gustavopch I hope so. As you can see, @timdorr currently tries to solve the merge conflicts that occurred lately.

@timdorr
Copy link
Member

timdorr commented Jun 15, 2020

I just did a simple conflict resolution. Looks like it needs some more work around the types, though.

@MeiKatz
Copy link
Contributor Author

MeiKatz commented Jun 15, 2020

@timdorr I will have a look at it tomorrow.

@shwao
Copy link

shwao commented Jul 3, 2020

Is there anything that could be done to get this merged? I would love to help but i cant see the conflict.

@MeiKatz
Copy link
Contributor Author

MeiKatz commented Jul 3, 2020

@shwao I have to fix it again. But it would be great if it could be merged before the repository is changed again.

@ashkan-pm
Copy link

Guys any progress on this? I'd be happy to help if there is anything I can do

@MeiKatz
Copy link
Contributor Author

MeiKatz commented Aug 12, 2020

So. I fixed the latest conflict. Please: merge it! I don't want to solve the same conflict again and again! @timdorr @mjackson @ryanflorence

let currentLocation = useLocation() as Location;
let usedLocation = location || currentLocation;

let matches = React.useMemo(() => matchRoutes(routes, usedLocation, basename), [
location,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really excited about this change @MeiKatz. One additional change: You’ll want to update the dependency array for useMemo to reference usedLocation instead of location so that it updates when that changes:

let matches = React.useMemo(() => matchRoutes(routes, usedLocation, basename), [
  usedLocation,
  routes,
  basename
]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I think I had this in but after all the edits I may have lost it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MeiKatz - perhaps you should force push an update to your PR with this ^ fix ?
@mjackson - I would love to see this get merged; pending your approval... I would like to start adding easy transitions to my routes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThomasBurleson the problem is, that I fix this pull request everytime there is a change in the code of others. And as long as nobody wants to merge my code I am unwilling to update it again and again.

@mjackson @ryanflorence @timdorr give me a sign if you want my pull request to be merged. Then I will fix it again and we can merge it. This pull request is open for too long and I want this story to have an end in the near future. Please be so kind.

@stale
Copy link

stale bot commented Nov 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
You can add the fresh label to prevent me from taking any action.

@stale stale bot added the stale label Nov 20, 2020
@stale stale bot closed this Nov 27, 2020
@MeiKatz
Copy link
Contributor Author

MeiKatz commented Nov 28, 2020

Please add fresh tag!

@timdorr timdorr added fresh and removed stale labels Nov 28, 2020
@timdorr timdorr reopened this Nov 28, 2020
@lwyj123
Copy link

lwyj123 commented Jan 3, 2021

As I mentioned in #7117

Same issues here, I was trying to add a page transition for my webapp, seems that the old example like this doesn't works for react-router v6. What's the solution in v6?

There are some problems for page transition and I really need it. Could @timdorr @ThomasBurleson take a look at this MR again?

@gustavopch
Copy link

@timdorr Is it really necessary to wait for @mjackson and @ryanflorence even though v6 is still currently in beta? I mean, everybody knows it's not stable and that breaking changes may occur until the stable release. It seems it's more harmful to hold this PR than to release it and start receiving any possible issues from users ASAP.

@timdorr
Copy link
Member

timdorr commented Jan 4, 2021

It's more that I know MJ and Ryan have made their own internal changes while developing Remix, so merging and releasing changes to the library is just going to cause conflicts and reverts later on. Unfortunately, they have been solely focused on Remix, so they haven't yet shared those changes, which would allow us to start merging changes here. It's a bit of BDFL situatiion, so we're at their whim.

@gustavopch
Copy link

gustavopch commented Jan 4, 2021

@timdorr Thanks for the explanation.

So I guess the best, for now, is to just use a fork.

@gnapse
Copy link

gnapse commented Jan 12, 2021

So I guess the best, for now, is to just use a fork.

Or using patch-package. I'm going to attempt that.

Indeed, this situation sucks. But I'm sure it will all be better when some of the goodies from Remix come back to react-router. Hopefully soon!

@mkaizad
Copy link

mkaizad commented Jan 16, 2021

Thanks for suggesting patch-package @gnapse, it's a great tool.

@JamesGelok
Copy link
Contributor

@MeiKatz You might've already seen this but @mjackson just tweeted about a potential stable release soon.

So maybe if @mjackson, @ryanflorence, @timdorr are available to give a sign that it's ready to be merged, you could resolve the conflict (possibly with @chris-lock's useMemo dependency suggestion) and potentially they could include it in the next stable release so we can finally natively use animated transitions between routes again.

If not, it could be a good addition to the next react-router-dom@next after stable release.

@MeiKatz
Copy link
Contributor Author

MeiKatz commented Jul 8, 2021

I don't know why, but when I try to check possible merge conflicts here in GitHub then I only get the loading spinner of eternity. Can somebody tell me if my PR has any conflicts? I then would fix them so this PR could be merged (after a looooong time pending).

@JamesGelok
Copy link
Contributor

@MeiKatz

packages/react-router/__tests__/Routes-test.js
packages/react-router/__tests__/useRoutes-test.js
packages/react-router/index.tsx

@MeiKatz
Copy link
Contributor Author

MeiKatz commented Jul 8, 2021

So. The pull request is up to date. Hopefully this was the last time I had to do it. So: please merge it!

@timdorr
Copy link
Member

timdorr commented Jul 8, 2021

Doesn't look like it at the moment. Try a rebase against the latest dev branch.

- the 2nd parameter of useRoutes() is now an object expecting `basename` and `location` as options
- did a little refactoring of the tests for useRoutes() and <Routes />
@MeiKatz
Copy link
Contributor Author

MeiKatz commented Jul 9, 2021

@timdorr Is it now correct?

@lwyj123
Copy link

lwyj123 commented Jul 9, 2021

@MeiKatz It seems like the CI is failed. you can check the action script to test locally.

@ryanflorence
Copy link
Member

Thanks for the work on this, and sorry for the BDFL, we'll be much more attentive here now that software is our sole focus.

Good news is, useRoutes takes the location arg:

locationArg?: Partial<Location> | string

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

Successfully merging this pull request may close these issues.

None yet