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

Call transition hooks when query changes for useQueries #2137

Merged
merged 1 commit into from
Oct 6, 2015

Conversation

schnerd
Copy link

@schnerd schnerd commented Sep 30, 2015

Fixes #2122

Any time the query params change, onLeave and onEnter hooks will both be called.

If comparing state.location.query instead of state.location.search would be more appropriate, let me know and I can adjust the PR–state.location.search was just easiest since it's a simple string comparison.

@mjackson
Copy link
Member

mjackson commented Oct 6, 2015

Thanks for the PR, @schnerd! Would you mind rebasing on current master? The tests should pass now.

@schnerd schnerd force-pushed the query-trigger-hooks branch from aa935a2 to 6161e21 Compare October 6, 2015 01:04
@schnerd
Copy link
Author

schnerd commented Oct 6, 2015

@mjackson think we're good to go 👍

knowbody added a commit that referenced this pull request Oct 6, 2015
Call transition hooks when query changes for useQueries
@knowbody knowbody merged commit 0f5b493 into remix-run:master Oct 6, 2015
@sikanhe
Copy link

sikanhe commented Oct 27, 2015

uh onEnter should definitely not be called here. Query params usually indicate the state of the current route/component, does not mean entering a new Route/Page.

This leads to undesired behavior such as page scrolling to the top when query param changes. Let's say you are using params/query params as state for the current tab, or a search box half way through the page. Whenever a push/replace state happens the page jumps back up.

@th0r
Copy link

th0r commented Oct 27, 2015

@sikanhe On the other hand you may want ?page=N query param to represent the current page in some items list and do want the page to scroll to top in this case.

@sikanhe
Copy link

sikanhe commented Oct 27, 2015

@th0r but isn't it much harder to manually program preserve scrolling than to scroll to the top? Scroll to the top is just window.scrollTo(0,0). On the other hand, it's much harder to do a one liner for "make sure to save the current position at x, y when query param changes". Correct me if i am wrong

@refast
Copy link

refast commented Oct 27, 2015

Perhaps, it should be done by analogy with React's componentWillMount (onRouteEnter) & componentWillUpdate (onRouteChange).

@taion
Copy link
Contributor

taion commented Oct 27, 2015

@refast Let's discuss this on #2332.

@schnerd schnerd deleted the query-trigger-hooks branch October 28, 2015 02:21
@taion taion mentioned this pull request Nov 13, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 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.

7 participants