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

shouldUpdateScroll must be component instance hook #439

Closed
klimashkin opened this issue Oct 30, 2014 · 5 comments
Closed

shouldUpdateScroll must be component instance hook #439

klimashkin opened this issue Oct 30, 2014 · 5 comments

Comments

@klimashkin
Copy link

Hello!
I have Route which handle photo gallery. This gallery have many parameters: page number (url parameter), filter and scroll (query parameters)
For example: /gallery/5?filter=1&sort=-5 means, fifth page, some filter and some descending sort
So, when user change page, he expect scroll change like browser page navigation, but when he toggle buttons for filter or sort, scroll should not be changed.

Variants of parameter combinations can be many different, so need a hook shouldUpdateScroll, which will be called after willTransitionTo and componentWillRecieveProps, when something in uri changes.

As I understand, the new variable ignoreScrollBehavior will not help me in described case

@klimashkin klimashkin changed the title shouldUpdateScroll must be component instance method shouldUpdateScroll must be component instance hook Oct 30, 2014
@ryanflorence
Copy link
Member

interesting, either that, or toss query/params/route info to the scroll behavior and you can write a scroll behavior that factor's this stuff in.

@gaearon
Copy link
Contributor

gaearon commented Nov 17, 2014

I also need this to support :userId/posts/?limit=number.

Can we make it that changing just query implicitly (and always) opts you out of scroll behavior?

I find it hard to image a case where you want to change query but reset scroll. Even if you need this, you can always promote your query to be a proper parameter, and then you have finer control.

gaearon added a commit to gaearon/react-router that referenced this issue Nov 26, 2014
Previously, the only way to opt out of scroll updates for a route would be by using `ignoreScrollBehavior`.
This, however, made it hard to implement arguably the most common use case: resetting scroll when `params` change and preserving it when only `query` changes.

This commit completely disables scroll updates when only `query` has changed.
This provides a reasonable default behavior and leaves `ignoreScrollBehavior` for more complicated cases.

If you'd rather keep the old behavior and reset scroll on query changes, you can either promote `query` variables to be route `params` or reset scroll position yourself in response to `query` changes in route handler's `componentDidUpdate`.

Fixes remix-run#432, remix-run#439
@mjackson
Copy link
Member

@gaearon Is this fixed?

@gaearon
Copy link
Contributor

gaearon commented Nov 26, 2014

@mjackson With #522, we don't update scroll on query-only changes.

@mjackson
Copy link
Member

@klimashkin I believe #522 addresses your original use case. When the route params change, the scroll is updated. When only the query changes, we don't touch the scroll. Please feel free to continue the conversation and we can re-open this issue if you need something more. :)

@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants