Update scroll restoration logic in suspense fixture

[hamlim]

This change should fix the issue raised in #13436.

The extra check of ensuring that showDetail has changed (i.e. prevState.showDetail !== this.state.showDetail) might not be necessary, however I wanted to make sure we don't call scrollTo too frequently.

Before submitting a pull request, please make sure the following is done:

1. Fork the repository and create your branch from master.
2. Run yarn in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (yarn test). Tip: yarn test --watch TestName is helpful in development.
5. Run yarn test-prod to test in the production environment. It supports the same options as yarn test.
6. If you need a debugger, run yarn debug-test --watch TestName, open chrome://inspect, and press "Inspect".
7. Format your code with prettier (yarn prettier).
8. Make sure your code lints (yarn lint). Tip: yarn linc to only check changed files.
9. Run the Flow typechecks (yarn flow).
10. If you haven't already, complete the CLA.

Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html

[sophiebits]

Let's see. It seems to me that we should scroll to top when going:

• from (currentId: 2, showDetail: false) to (currentId: 2, showDetail: true)
• from (currentId: 2, showDetail: true) to (currentId: 2, showDetail: false)
• from (currentId: 2, showDetail: true) to (currentId: 3, showDetail: true)

But not when going:

• from (currentId: null, showDetail: false) to (currentId: 2, showDetail: false)

Do you agree with that? Let me know if not.

[plievone]

@sophiebits Extra points for doing proper scroll restoration, such as:

• store the state in url by using window.history.pushState when available
• concatenating pathname, search and hash from window.location to get a location key
• saving it as a this.previousLocation in each render (or which ever works in async land as componentWillUpdate is deprecated), and comparing it with current location
• whenever a change is detected, save the window.pageXOffset and window.pageYOffset with the key to a limited structure in sessionStorage
• in componentDidUpdate (when DOM is correctly laid out) compare the prevLocation and location, and if changed, get the location with the key from sessionStore and set window.scrollTo(pos.x, pos.y), or if key is missing, only then use the scroll to top.
  ...because then this suspense demo could be an important example for not breaking the back/forward button?

[hamlim]

@sophiebits Yep those conditions sound correct to me.

I initially thought that the second and third bullet points in the cases where we should restore scroll wouldn't be possible states. However, if the UI changes for example the "Return to List" button becoming sticky at the top of the window, or next and previous buttons to navigate between team members get added then we would want to still support those cases.

With those in mind it seems like the logic should be something like

• if showDetail changed (either from false => true in case 1, or true => false in case 2) or,
• if currentId changes and showDetail is now true (case 3)

if (
  prevState.showDetail !== this.state.showDetail ||
  (prevState.currentId !== this.state.currentId && this.state.showDetail)
) {
  // restore scroll
}

Let me know if this updated snippet seems to cover those cases correctly.

@plievone I think your solution would work really well, however it feels a bit like over-engineering a solution in my opinion.

I find it really nice to see these fixtures as simple as they are at the moment to help developers understand the core concept of what they are showcasing. On top of testing the feature they have the added benefit of teaching new developers how to implement the feature as well.

Happy to discuss more about it though!

[sophiebits]

@hamlim That sounds good to me!

[sophiebits]

Looks great! Thanks!

[hamlim]

Awesome, thanks again for the issue, and the feedback!