Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

improve scrolling to element #1072

Closed
wants to merge 1 commit into from
Closed

improve scrolling to element #1072

wants to merge 1 commit into from

Conversation

stipsan
Copy link
Contributor

@stipsan stipsan commented Aug 5, 2018

Saw this tweet and thought I'd give it a go: https://twitter.com/aweary/status/1025789362607255552

Related issue: #746

kapture 2018-08-05 at 22 06 22

This PR adds support for horizontal scrolling and also ensures that vertical scrolling is always in view. There's room for improvements though:

  • There's a setTimeout workaround for when the height of the elements scrolling frame changes because of very long multi-row breadcrumbs to ensure things are scrolled into view correctly.
  • Some things like textNodes have a very wide width triggering unnecessary scrolling.
  • There's little space between the edges and the node being scrolled to, but I'll be adding support for scroll-margin CSS in scroll-into-view-if-needed soon which will allow changing that.

Benefits to using the scroll-into-view-if-needed ponyfill, and later possibly scroll-margin:

  • Standards based approach that allows removing dependencies and custom logic in the future when browser support is good enough.
  • Less code to maintain in this project.

Thoughts? 😃

@bvaughn
Copy link
Contributor

bvaughn commented Aug 20, 2018

Hi @stipsan! Thanks so much for the contribution!

Sorry for not seeing this PR sooner. 🙁 I fixed this in a slightly different way via #1091 by using scrollIntoView rather than scrollIntoViewIfNeeded since the container elements are wide enough to fool the browser into thinking they're in view when the content we want to see isn't actually.

So I think this PR can be closed! (Again, my apologies)

@bvaughn bvaughn closed this Aug 20, 2018
@stipsan
Copy link
Contributor Author

stipsan commented Aug 21, 2018

No worries at all @bvaughn!

Awesome fix btw, less polyfills less problems 🎉

@stipsan stipsan deleted the fix-scrolling-to-element branch August 21, 2018 09:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants