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(nuxt): improve default router scrollBehaviour #24960

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bernhardberger
Copy link
Contributor

@bernhardberger bernhardberger commented Dec 29, 2023

handle first page load and page refresh scenarios (restore scroll position without jumps), fixes hash-navigation when changing pages and browser back/forward positions, especiall when page transitions are involved.

🔗 Linked issue

fixes #24941
fixes #22487
fixes #19664
fixes #25030

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

handle first page load and page refresh scenarios (restore scroll position without jumps), fixes hash-navigation when changing pages and browser back/forward positions, especially when page transitions are involved.

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Dec 29, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bernhardberger
Copy link
Contributor Author

fyi, fixed the typescript errors and force pushed my branch to avoid commit clutter

Comment on lines +74 to +59
// Without this setTimeout, the scroll behaviour is not applied, this is however working reliably
// todo: figure out how we can solve this without setTimeout of 0ms
Copy link
Member

Choose a reason for hiding this comment

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

Maybe using nextTick twice here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, unfortunately it doesn't help. I even tried chaining multiple nextTicks() here and it didn't help. I think it has to do with what's happening when using setTimeout: https://medium.com/@sohnu/settimeout-with-time-0-what-does-it-really-mean-3b306880a0f6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should have added the quote I'm referring to, sry:

But what we have to see here is, all the 3 setTimeouts are taken away from the actual call executions and when there is nothing in the actual call stack, the setTimeouts started to execute with the time specified.

So whenever we want to take the call outside of the flow and run it once after the call stack is free, we can use setTimeout with time 0.

tbh, I have absolutely no clue why this is fixing it. I'm happy for any theory as to what's going on here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran into an issue that had the same sort of resolution. I don't really understand what it is about nextTick that's not working in every case, but I opened a PR that might be relevant to this discussion: #25817

@bernhardberger bernhardberger force-pushed the fix/default-scroll-behaviour branch 4 times, most recently from dc9c381 to 2f1d8b6 Compare January 7, 2024 21:36
handle first page load and page refresh scenarios (restore scroll position without jumps), fixes hash-navigation when changing pages and browser back/forward positions, especiall when page transitions are involved. fixes nuxt#24941, nuxt#22487, nuxt#25030 and nuxt#19664

Signed-off-by: Bernhard Berger <bernhard.berger@gmail.com>
@danielroe danielroe added the bug label Jan 14, 2024
@danielroe danielroe changed the title fix: improved default router.options scrollBehaviour fix(nuxt): improve default router.options scrollBehaviour Jan 16, 2024
@danielroe danielroe changed the title fix(nuxt): improve default router.options scrollBehaviour fix(nuxt): improve default router scrollBehaviour Jan 16, 2024
@danielroe
Copy link
Member

/trigger release

Copy link
Contributor

🚀 Release triggered! You can now install nuxt@npm:nuxt-nightly@pr-24960

@danielroe
Copy link
Member

Would you be able to update the PR following #25483? 🙏

And thank you so much for this ❤️

@bernhardberger
Copy link
Contributor Author

Would you be able to update the PR following #25483? 🙏

And thank you so much for this ❤️

I'm a bit limited on time this week. Hopefully I can get to it next week.

@danielroe danielroe marked this pull request as draft February 4, 2024 20:34
@danielroe
Copy link
Member

Marking as draft just for triage - feel free to unmark whenever it's ready from your end 🙏

@bernhardberger
Copy link
Contributor Author

bernhardberger commented Apr 15, 2024

Would you be able to update the PR following #25483? 🙏
And thank you so much for this ❤️

I'm a bit limited on time this week. Hopefully I can get to it next week.

Turns out I've been drowned in work and family stuff ever since then. I have not abandoned it, it's still on my mind, however I just have zero spare time at the moment.

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