-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 invalid rendering in scroll synced list with height/width exceeding browser limit #727
Conversation
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.
This looks great. Thanks!
How'd you spot/fix all my little typos in docs comments. Do you have some sort of tool that catches those? I'd love to know about it.
@@ -37,7 +37,7 @@ export default function defaultCellRangeRenderer ({ | |||
rowSizeAndPositionManager.areOffsetsAdjusted() | |||
) | |||
|
|||
const canCacheStyle = !isScrolling || !areOffsetsAdjusted | |||
const canCacheStyle = !isScrolling && !areOffsetsAdjusted; |
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.
nit: This project uses standard (no semis)
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.
whoops! will update
I installed a simple spell checker plugin for vscode a couple of weeks ago, it's working pretty well. |
Just realized that my test is not testing what I thought it was, will revisit that too. |
That's awesome! Clearly I need to install the same plug-in!
Okay! No hurry. Let me know when it's ready to re-review. 😄 |
mmmm... this change breaks some other tests, I'm not clear on whether those tests were relying on incorrect behavior or not |
Thank you for this bug-fix! This feature has just been released in version 9.9.0 of the library and I've given you credit in the CHANGELOG. |
See issue: #725
It seems that when syncing two or more scrollable lists whose total height exceeds DEFAULT_MAX_SCROLL_SIZE here: https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/utils/ScalingCellSizeAndPositionManager.js#L9, the cell offset calculation does not produce correct results.
Here is a fiddle demonstrating the problem: https://jsfiddle.net/nathanpower/823w11xr/1/
Note that using the scroll-bar is fine, but using a scroll wheel or two-finger trackpad scroll results in messed up rendering of the synced list.
This PR changes to only caching the style when not scrolling AND offsets are not adjusted. Maybe this was the original intention here?
Also some minor typos are corrected and
.vscode
folder added to.gitignore
, hope that's ok.