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(scrollBehavior): trigger scroll behavior if same route with hash #3592

Merged
merged 3 commits into from
Oct 8, 2021

Conversation

ryanklarhoelter
Copy link
Contributor

@ryanklarhoelter ryanklarhoelter commented Jul 12, 2021

close #1668

This will make clicks on already active anchor links work.

@ryanklarhoelter
Copy link
Contributor Author

ryanklarhoelter commented Jul 12, 2021

Removing the whole "isSameRoute" condition would also do the trick (as @gspain found out: #1668 (comment)). But I'm concerned that this would have some unwanted side effects that I'm not aware of.

@ryanklarhoelter
Copy link
Contributor Author

ryanklarhoelter commented Jul 12, 2021

Because this would have an effect on all the current implementations I could also think of an option for the scroll behavior that developers could set to true—and only then the scroll behavior for same routes would be triggered.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Thanks for this! Can you add an e2e test and make sure handleScroll is only called if there is a hash?

@ryanklarhoelter ryanklarhoelter requested a review from posva October 7, 2021 14:07
@ryanklarhoelter
Copy link
Contributor Author

Hi @posva! I guess we misunderstood a little. I've added a test now that checks if clicking the same anchor twice works (scrolls to the anchor location again). But: I haven't added a test that checks if "handleScroll()" is called for URLs with hashes only. I think it's useful to call "handleScroll()" whenever a link is clicked that doesn't change the URL. That will make multiple clicks on the same anchor work out of the box—plus it gives the developer the chance to implement an individual behavior (by customizing the scroll behavior) for clicking a link to the current site again (when there is no hash). They could decide for example to scroll to the top of the page or to reload the entire page (as it happens for simple HTML pages). What do you think?

@posva
Copy link
Member

posva commented Oct 7, 2021

It has to be hash only because otherwise this would break existing scrollBehavior() for users: right now clicking the link we are on does nothing but, in most cases (scroll to top), with the new changes, that would scroll to the top instead.

In Vue Router 4, scrollBehavior already works like you suggested.

@ryanklarhoelter ryanklarhoelter changed the title fix(scrollBehavior): trigger scroll behavior if same route fix(scrollBehavior): trigger scroll behavior if same route with hash Oct 8, 2021
@ryanklarhoelter
Copy link
Contributor Author

Done! 👍

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Thanks!

@posva posva merged commit 57d8042 into vuejs:dev Oct 8, 2021
@HenrijsS
Copy link

HenrijsS commented Oct 8, 2021

Judgement day is here. It's finally fixed!!!

@paulolieuthier
Copy link

When can we expect this to be released?

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.

Navigation to the same route fails (anchor, hash)
4 participants