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

[changed] Skip scroll update if only query has changed #477

Closed

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Nov 17, 2014

This is supposed to fix #432, #439.

@mjackson @rpflorence what do you think?


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.

@gaearon gaearon force-pushed the never-update-scroll-on-query-changes branch from 3023b97 to 324715d Compare November 17, 2014 23:45
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`.
@gaearon gaearon force-pushed the never-update-scroll-on-query-changes branch from 324715d to b530d92 Compare November 18, 2014 00:02
@mjackson
Copy link
Member

@gaearon This looks great. I agree the default should be to not update the scroll position when only the query has changed.

We have been doing a ton of work on the new-api branch, and we're planning on shipping it very soon (hopefully this weekend). Right now the branch is missing ignoreScrollBehavior entirely (still need to get it in, it was a major refactor) but I'd like to get this in as well.

I'd be happy to integrate this work as well, but was wondering if you might have some time to do it. The underlying arch is very similar to what we had before. Mostly high-level API has changed.

@gaearon
Copy link
Contributor Author

gaearon commented Nov 21, 2014

@mjackson

Unfortunately I'm going to be pretty busy weekend so not sure. I'll have some time next week so if initial new API release doesn't include this, I can bring it back. Let me know!

@ryanflorence
Copy link
Member

Thanks, I say ship new api without the feature if none of us get to it and
then bring it in later

On Friday, November 21, 2014, Dan Abramov notifications@github.com wrote:

@mjackson https://github.com/mjackson

Unfortunately I'm going to be pretty busy weekend so not sure. I'll have
some time next week so if initial new API release doesn't include this, I
can bring it back. Let me know!


Reply to this email directly or view it on GitHub
#477 (comment).

@gaearon
Copy link
Contributor Author

gaearon commented Nov 21, 2014

Yeah I think people will spend some time porting to new API anyway so we get some time to bring it in

@gaearon
Copy link
Contributor Author

gaearon commented Nov 21, 2014

New API is genius btw. That reduce example makes me break down in tears. First-class nesting shows its true power.

@mjackson
Copy link
Member

ok, @gaearon. I didn't want you to feel like we were ignoring you :) Let's go ahead and cut a release and get scrolling worked out next week, then cut another stable.

@gaearon gaearon closed this Nov 26, 2014
@gaearon gaearon deleted the never-update-scroll-on-query-changes branch November 26, 2014 15:44
@gaearon
Copy link
Contributor Author

gaearon commented Nov 26, 2014

Superseded by #522

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 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

Successfully merging this pull request may close these issues.

Preserving scroll position when changing path query
3 participants