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

Scroll when container contracted to hide current scroll #798

Merged

Conversation

ReinAkane
Copy link
Contributor

Fixing issue #780: scrolling then reducing list size could cause all items to disappear in Firefox. See issue for a GIF.

The root cause is that Firefox does not fire a scroll event when a scrolled element's size decreases. This causes the Grid's scrollTop and scrollLeft values to be out of sync with the actual scroll location of the element.

This PR makes the Grid check for item count changes to determine if it needs to scroll the grid back into view.

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.

Haven't tested this. I trust you. 😄

@bvaughn bvaughn merged commit b7ae226 into bvaughn:master Sep 18, 2017
@bvaughn
Copy link
Owner

bvaughn commented Sep 18, 2017

This feature has been released in version 9.10.0. Thank you for contributing!

@ReinAkane
Copy link
Contributor Author

@mcordova47 I'm not going to have time to test for a while, sadly. If you want to pull it down and test yourself, I added unit tests to ensure that the bug this PR fixed continues to be fixed.

@bvaughn
Copy link
Owner

bvaughn commented Sep 23, 2017

I think this introduced the regression described in #816 and bvaughn/react-virtualized-select/issues/75

Happy to review a fix if you'd like to put up a PR @mcordova47

@mcordova47
Copy link
Contributor

@bvaughn @ReinAkane I looked a little further into it, and the problem I am facing is a little different than the problem described in #816. It seems to be a problem with the MultiGrid component that was exposed by this commit. MultiGrid has no default value for scrollToRow, and it sets its bottom right grid's scrollToRow to props.scrollToRow - props.fixedRowCount, causing it to be NaN when it should default to -1.

Adding defaults of -1 to scrollToRow and scrollToColumn in MultiGrid seems to fix my issue. I created a pull request (#820), if you'd like to review that. Some tests for other components were failing before my fix, but all tests for MultiGrid pass. Added two tests that were failing before the fix.

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.

3 participants