-
Notifications
You must be signed in to change notification settings - Fork 90
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
Allow containers other than window to scroll #41
base: master
Are you sure you want to change the base?
Conversation
duration: this.config.scrollDuration, | ||
offset: this.config.offset, | ||
}) | ||
this.config.scroller.center(element, this.config.scrollDuration, offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about why use scroller.center
+ this.config.offset + viewHeight/2
here?
If element's height is larger than container, it will not show the top of the element, but maybe we want to see the element from the beginning of it.
Maybe there is another solution? @dbramwell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this 10 months ago, so the details are a bit fuzzy. If you look at the demo, it contains elements that are larger than the container and it does work as expected. I think I did it this way because of the way zenscroll works. From the docs:
"If you want you can also define an offset. The top of the element will be upwards from the center of the screen by this amount of pixels."
So, with an offset of zero, the top of the element would end up in the centre of the container. We want the top of the element to be at the top of the container so we need an offset of half the container's height (plus any offset requested in the config).
Does that answer the question?
setContainer = () => { | ||
// if we have a containerId, find the scrolling container, else set it to window | ||
if (this.config.containerId) { | ||
this.config.container = document.getElementById(this.config.containerId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this.config.containerId
could be id's string or container's ref ?
As it is possible that parent component which is using ScrollableAnchor
may maintain a container's ref~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a valid request I guess. I don't think this library is really maintained any more so it's unlikely this will ever get merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a version of this library to my library collection that has some added improvements already (Sadly no documentation yet, i'm working on it though)
https://github.com/leon-marzahn/marzahn-dev/tree/main/libs/react/scroll-anchor
Addresses issue 4 by building upon pull 37
See example 5 here
Edit:
I've now pushed a release of this into a branch on my github fork. Hopefully this will be merged soon, but for now the new feature can be used by adding the following to the dependencies in your package.json:
"react-scrollable-anchor": "github:dbramwell/react-scrollable-anchor#release"