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 broken hash scroll logic #4766

Merged
merged 4 commits into from
Aug 11, 2018
Merged

Fix broken hash scroll logic #4766

merged 4 commits into from
Aug 11, 2018

Conversation

oliviertassinari
Copy link
Contributor

@oliviertassinari oliviertassinari commented Jul 11, 2018

<Container> does not receive any property. There is no way the scrollToHash logic can work right now. I believe it's a regression. It was working fine at some point. I'm sorry, I'm too lazy to add a test.

This fix was tested on Material-UI 👌.

This bug reproduction is the following:
As soon as you want to transition to a new page with a hash. The scroll doesn't change.

  • start on pageA
  • you scrollTop to 100
  • you move to pageB#hash
  • you stay at scrollTop 100, but #hash is at scrollTop 400.

@timneutkens
Copy link
Member

Thanks for the PR! Can you add a test here: https://github.com/zeit/next.js/blob/1a3f9507773a43e44748cbe5c0017223b99c3980/test/integration/basic/test/client-navigation.js#L246-L270

So that we can never regress on this again.

`<Container>` does not receive any property. There is no way the *scrollToHash* logic can work right now. I believe it's a regression. It was working fine at some point on Material-UI.
@oliviertassinari oliviertassinari changed the base branch from master to canary July 15, 2018 10:10
@oliviertassinari
Copy link
Contributor Author

oliviertassinari commented Jul 15, 2018

I have added a test. For some reason, it's green even without my fix 😱. I don't have a definitive explanation. It could be linked to rendering large pages vs the test pages that are almost empty.

You can reproduce the bug live on https://material-ui.com/ clicking on:
capture d ecran 2018-07-15 a 12 13 21

@oliviertassinari
Copy link
Contributor Author

@timneutkens What do you think of updating the logic just for the sake of going back to what we had before the regression? This issue would require more investigation from me, but it's not something I have the budget for.

@timneutkens
Copy link
Member

I'll try to reproduce, otherwise, the logic can be removed. As I was expecting it could be removed before already.

@timneutkens timneutkens merged commit b02fff6 into vercel:canary Aug 11, 2018
@timneutkens
Copy link
Member

timneutkens commented Aug 11, 2018

Just going to merge it in right now to be back at the previous behavior. Also, the test seems fine 👍

@oliviertassinari oliviertassinari deleted the patch-5 branch August 11, 2018 20:19
@oliviertassinari
Copy link
Contributor Author

@timneutkens Thank you 🙏

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

2 participants