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

Fix invalid rendering in scroll synced list with height/width exceeding browser limit #727

Merged
merged 3 commits into from
Jul 9, 2017

Conversation

nathanpower
Copy link

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.

Copy link
Owner

@bvaughn bvaughn left a 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;
Copy link
Owner

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops! will update

@nathanpower
Copy link
Author

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.

I installed a simple spell checker plugin for vscode a couple of weeks ago, it's working pretty well.

@nathanpower
Copy link
Author

Just realized that my test is not testing what I thought it was, will revisit that too.

@bvaughn
Copy link
Owner

bvaughn commented Jul 7, 2017

I installed a simple spell checker plugin for vscode a couple of weeks ago, it's working pretty well.

That's awesome! Clearly I need to install the same plug-in!

Just realized that my test is not testing what I thought it was, will revisit that too.

Okay! No hurry. Let me know when it's ready to re-review. 😄

@nathanpower
Copy link
Author

mmmm... this change breaks some other tests, I'm not clear on whether those tests were relying on incorrect behavior or not

@bvaughn bvaughn merged commit fe3a3cb into bvaughn:master Jul 9, 2017
@bvaughn
Copy link
Owner

bvaughn commented Jul 9, 2017

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.

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.

2 participants