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

feat: Opt out scrolling upon a hash change #1105

Conversation

elliottgrieco-toast
Copy link

@elliottgrieco-toast elliottgrieco-toast commented Jan 29, 2024

Discussion as a feature in #1155

Adds ability to opt out of behavior where the router (specifically, the DOM/UI library implementation) handles scrolling to hash element matching the current hash history.

I am driven to add this new router option because I am currently working with an existing code-base which controls the current router hash location based on the browser's current scroll position.

A minimum reproduction case where the scrollIntoView behavior conflicts with the desired behavior to control the router location externally is located at:

https://stackblitz.com/edit/tanstack-router-cyyfgk?file=src%2Fmain.tsx

@elliottgrieco-toast elliottgrieco-toast changed the title Add option to prevent scrolling upon a hash change feat: Add option to prevent scrolling upon a hash change Jan 29, 2024
@elliottgrieco-toast elliottgrieco-toast changed the title feat: Add option to prevent scrolling upon a hash change feat: Opt out scrolling upon a hash change Feb 6, 2024
@Iessenti
Copy link

Hi! Can we also add a similar configuration flag to your PR that allows you to scroll to the hash smoothly? And can you tell me why your PR hasn't been reviewed yet?

@SeanCassiere
Copy link
Contributor

SeanCassiere commented Mar 25, 2024

Been going through our open PRs, and regarding this one, I've got a question about why this implementation is required.

This functionality can be achieved using the getKey callback function that you can pass into the <ScrollRestoration> component.
It has the added benefit of being customizable by the end user to suit their exact scroll restoration requirements.

In this case, you could use to opt-out of a hash change triggering the scroll restoration.

<ScrollRestoration
  getKey={location => {
    return [location.pathname, location.search].join(",")
  }}
/>

Perhaps, we can include a section in the Scroll Restoration documentation on some popular recipes for using the getKey callback to get this behaviour and others.

@elliottgrieco-toast
Copy link
Author

Thanks for the thoughtful consideration!

Been going through our open PRs, and regarding this one, I've got a question about why this implementation is required.

It looks like in your comment you are proposing a way to use the ScrollRestoration API in order to mimic the behavior of scrolling on hash router change?

Thanks for pointing me towards the Scroll Restoration capabilities documentation, as this revealed a new thought to perhaps approach this issue as an 'opt in' capability.

I.e., using the useElementScrollRestoration hook and while removing the existing scrollIntoView behavior:

useElementScrollRestoration(
  getElement() { return document.getElementById(router.location.hash) }
  getKey(location) { return [location.pathname, location.search, location.hash].join(",") }
)

However, this would present the following difficulties vs. adding a new router configuration option:

  1. This would opt users out of a likely 'sensible default' behavior to scroll users to the hash on route changes, especially after transitions complete.
  2. This would probably be considered a breaking change, whereas adding the proposed scrollOnHashChange option would be considered a minor, additive change to the RouterOptions API.
  3. I agree that requiring opting in would require improved patterns documentation for situations like this; if this part of documentation continues to become enhanced, perhaps removing the hash-based scrollIntoView functionality from the RouterProvider would indeed be more consistent with the TanStack's broader behavior for leaving the complexities of scroll state management to the user. I am certainly willing to assist in this as part of the feature development.
  4. It is perhaps worth looking into the broader resliency of the useElementScrollRestoration API in general before encouraging user's to further leverage it for advanced use-cases; I wonder if there are any potential gotchas with exposing this functionality to router users, such as whether it's possible to easily create conflicting scroll handlers. The spirit of the hook makes a lot of sense, but it's potential to fragment rather than co-locate side-effects on an important piece global state like scroll position perhaps lends itself to being located in a more centralized location (e.g. as a more customizable, albeit more complex RouterOptions property).

@SeanCassiere
Copy link
Contributor

I believe I'm understanding what you are going for.

But I'm a bit hesitant about widening our API surface unless we absolutely need to and as such I want to make sure we've covered the alternates before moving to adding a new piece of configuration to the Router as well as how it'd play along with the existing implementation and ScrollRestoration component.

Especially since something like scroll restoration behaviour could/would eventually need to become a per-route based configuration option as people start exploring this avenue.

With that in mind:

  1. Is any of this possible using the ScrollRestoration component? or does this have to be a Router configuration option?
  2. Are you still basing your need for this configuration based on the original example? Or is it tied to something broader? (if so what is it)
  3. What are the impacts of using it with existing navigate and ScrollRestoration?

I've tweaked the main.tsx file from your original example, to use the navigate calls we'd see in a normal TSR app, and it does seem to cause some wonky behaviour scroll behaviours.

I hope the tweaked example helps you understand my hesitancy here.

@elliottgrieco-toast
Copy link
Author

But I'm a bit hesitant about widening our API surface unless we absolutely need to and as such I want to make sure we've covered the alternates before moving to adding a new piece of configuration to the Router as well as how it'd play along with the existing implementation and ScrollRestoration component.

AH, now I understand your motivations. I'm certainly motivated by an edge case where the code performing external hash changes is completely outside my own codebase, so I definitely own that that's an edge case that TS router shouldn't design around.

Others are liking and thumbs upping this idea still, so I'm wondering the other use-cases that are motivating folks.

I'll play around with the Scroll restoration API as is if there's any way for it to override the "hard coded" hash scrolling behavior. What I suspect is that the way the opinion is so hard coded into the Provider, that there will be scroll conflicts.

Perhaps then, an alternative is to have the Provider defer to the location key as a source of truth vs the hash 🤔. Will play around, and work on top of assumption that the user desires the "update history" behavior and has it in their control, vs my current assumption of it being externally controlled and rigid.

@elliottgrieco-toast
Copy link
Author

I have followed up in the related issue with a call for more concrete use cases where the current default scroll behavior is problematic

#1155 (comment)

@SeanCassiere SeanCassiere marked this pull request as draft April 19, 2024 10:58
@SeanCassiere
Copy link
Contributor

@elliottgrieco-toast pinging for a checkup.

Checking the original discussion thread, discussions, and Discord questions, I haven't seen any movement on the need for this.

For navigation controlled outside of React, the people seem to be fine with consuming the router using its created instance or by using the useRouter hook.

@elliottgrieco-toast
Copy link
Author

Agreed -- an attempt was made; I'm ready to close this issue at your discretion.

@SeanCassiere
Copy link
Contributor

Closing this for now because of the discussion above.

Thanks for the contribution, @elliottgrieco-toast!

Appreciate your understanding on this.

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.

None yet

3 participants