-
Notifications
You must be signed in to change notification settings - Fork 59
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] Ensure valid changes on multi-scroll panes #8
Conversation
src/ScrollSync.js
Outdated
const { scrollTop, scrollHeight, clientHeight, | ||
scrollLeft, scrollWidth, clientWidth } = scrolledPane | ||
const { | ||
scrollTop, scrollHeight, clientHeight, |
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.
Can you please put every identifier on a new line?
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.
Thought its easier to read this way as the properties are grouped by context (...width
/ ...height
) but okay 😉
src/ScrollSync.js
Outdated
@@ -82,11 +87,11 @@ export default class ScrollSync extends Component { | |||
const paneHeight = pane.scrollHeight - clientHeight | |||
const paneWidth = pane.scrollWidth - clientWidth | |||
/* Adjust the scrollTop position of it accordingly */ | |||
if (vertical) { | |||
pane.scrollTop = proportional ? (paneHeight * scrollTop) / (scrollHeight - clientHeight) : scrollTop // eslint-disable-line | |||
if (vertical && scrollTopOffset) { |
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 think && scrollOffset > 0
will be more readable and less ambiguous or what this check is doing?
Thanks for the PR! |
@okonet Sure. Always glad to find the right tool as you're looking for a solution of a less common problem and the opportunity to contribute back 😄 |
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.
👍
WHAT
In the current implementation its possible that a division by
0
occurs if a sync container has multiple panes with different directions (at least one vertical and one horizontal). In this case some elements have nooverflow-x
oroverflow-y
value but a fixed value. To avoid collision a check is added to ensure only valid results will be set.Furthermore for backwards compatibility the previous
react
version was added to the peer dependencies.