Make scroll offset uncontrollable in favor of ref methods #1320

somi92 · 2019-02-05T09:32:28Z

Changes based on discussion from #999.

Summary of changes:

Grid component
- Make Grid uncontrollable, rename scrollTop/scrollLeft props defaultScrollTop/defaultScrollLeft (only to be used on mount)
- Limit scrollTop and scrollLeft to maximum possible values state update in scrollToPosition method(prevents issues when calling scrollToPosition multiple times with larger than max)
- Update tests and docs
MultiGrid component
- Remove scrollLeft and scrollTop from state
- Remove getDerivedStateFromProps and state handling code componentDidMount
- Update all _onScroll methods to use scrollToPosition for syncing
- Add scrollToPosition method (pass-thru that calls of the main scrolling grid which is _bottomRightGrid)
- Update tests and docs
List component
- Make List uncontrollable, rename scrollTop to (only to be used on mount)
- Update tests and docs
Table component
- Make Table uncontrollable, rename scrollTop to defaultScrollTop (only to be used on mount)
- Update tests and docs
ScrollSync component
- Add flow types
- Add registerChild to children render function parameters
- Implement _registerChild to store refs on scrollable children
- Call scrollToPosition on children refs in _onScroll handler sync them
- Update tests, docs and example
WindowScroller component
- Update flow types
- Rename existing registerChild method to registerChildContainer
- Add registerChild to children render function parameters
- Implement _registerChild to store ref on scrollable child
- Call scrollToPosition on child ref in _handleWindowScrollEvent
  handler
- Update tests, docs and example

Thanks for contributing to react-virtualized!

Before submitting a pull request, please complete the following checklist:

The existing test suites (npm test) all pass
For any new features or bug fixes, both positive and negative test cases have been added
For any new features, documentation has been added
For any documentation changes, the text has been proofread and is clear to both experienced users and beginners.
Format your code with prettier (npm run prettier).
Run the Flow typechecks (npm run typecheck).

Here is a short checklist of additional things to keep in mind before submitting:

Please make sure your pull request description makes it very clear what you're trying to accomplish. If it's a bug fix, please also provide a failing test case (if possible). In either case, please add additional unit test coverage for your changes. :)
Be sure you have notifications setup so that you'll see my code review responses. (I may ask you to make some adjustments before merging.)

* Grid component - Make Grid uncontrollable, rename scrollTop/scrollLeft props to defaultScrollTop/defaultScrollLeft (only to be used on mount) - Limit scrollTop and scrollLeft to maximum possible values before state update in scrollToPosition method(prevents rendering issues when calling scrollToPosition multiple times with values larger than max) - Update tests and docs * MultiGrid component - Remove scrollLeft and scrollTop from state - Remove getDerivedStateFromProps and state handling code from componentDidMount - Update all _onScroll methods to use scrollToPosition for syncing - Add scrollToPosition method (pass-thru that calls scrollToPosition of the main scrolling grid which is _bottomRightGrid) - Update tests and docs * List component - Make List uncontrollable, rename scrollTop to defaultScrollTop (only to be used on mount) - Update tests and docs * Table component - Make Table uncontrollable, rename scrollTop to defaultScrollTop (only to be used on mount) - Update tests and docs * ScrollSync component - Add flow types - Add registerChild to children render function parameters - Implement _registerChild to store refs on scrollable children - Call scrollToPosition on children refs in _onScroll handler to sync them - Update tests, docs and example * WindowScroller component - Update flow types - Rename existing registerChild method to registerChildContainer - Add registerChild to children render function parameters - Implement _registerChild to store ref on scrollable child - Call scrollToPosition on child ref in _handleWindowScrollEvent handler - Update tests, docs and example

wuweiweiwu · 2019-02-24T16:04:02Z

@somi92 Thanks for doing this! I'll take some time soon to thoroughly review this and test.

wuweiweiwu

A couple comment about how we're handing null checks & other small things.

This is AMAZING 💯

source/ScrollSync/ScrollSync.js

wuweiweiwu · 2019-05-03T08:20:59Z

source/ScrollSync/ScrollSync.js

+        "ScrollSync registerChild expects to be passed a component with 'scrollToPosition' function",
+      );
+    }
+    this._elements.push(element);


Should we register if its falsey?

You're right, probably not, there is no much sense in keeping the element in that case. I'll fix this!

somi92 · 2019-05-04T18:34:11Z

@wuweiweiwu Thanks! I'm a bit short on time for a couple of days but will respond to your remarks by Tuesday.

wuweiweiwu · 2019-05-06T01:52:35Z

@somi92 No problem! Take your time :)

Your effort on this has been very much appreciated

codecov-io · 2019-11-13T04:04:30Z

Codecov Report

Merging #1320 into master will decrease coverage by 0.26%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #1320      +/-   ##
==========================================
- Coverage    94.7%   94.44%   -0.27%     
==========================================
  Files          47       47              
  Lines        1795     1800       +5     
==========================================
  Hits         1700     1700              
- Misses         95      100       +5

Impacted Files	Coverage Δ
source/List/List.js	`78.37% <ø> (ø)`	⬆️
source/Table/Table.js	`90.35% <ø> (+1.75%)`	⬆️
source/Grid/Grid.js	`95.09% <100%> (-0.06%)`	⬇️
source/WindowScroller/WindowScroller.js	`92.94% <100%> (-5.8%)`	⬇️
source/ScrollSync/ScrollSync.js	`100% <100%> (ø)`	⬆️
source/MultiGrid/MultiGrid.js	`96.07% <90.47%> (-1.06%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7304ec6...02bf29a. Read the comment docs.

wuweiweiwu self-assigned this Feb 24, 2019

wuweiweiwu added this to the 10.0.0 milestone Feb 24, 2019

wuweiweiwu mentioned this pull request Mar 8, 2019

Fix case when scrollToPosition will not update internal state in React >= 16.4. #1288

Merged

6 tasks

wuweiweiwu reviewed May 3, 2019

View reviewed changes

wuweiweiwu added the breaking change label Nov 12, 2019

wuweiweiwu added 4 commits November 12, 2019 18:23

Merge branch 'master' into uncontrollable-scroll-offset

44abe8b

Update MultiGrid.js

bc9a4f8

Update ScrollSync.js

59a3263

Update ScrollSync.js

02bf29a

wuweiweiwu changed the base branch from master to hwu/uncontrollable-scroll-offset November 13, 2019 04:08

wuweiweiwu merged commit b247882 into bvaughn:hwu/uncontrollable-scroll-offset Nov 13, 2019

wuweiweiwu mentioned this pull request Nov 13, 2019

Make scroll offset uncontrollable in favor of ref methods #1461

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make scroll offset uncontrollable in favor of ref methods #1320

Make scroll offset uncontrollable in favor of ref methods #1320

somi92 commented Feb 5, 2019

wuweiweiwu commented Feb 24, 2019

wuweiweiwu left a comment

wuweiweiwu May 3, 2019

somi92 May 8, 2019

somi92 commented May 4, 2019

wuweiweiwu commented May 6, 2019

codecov-io commented Nov 13, 2019 •

edited

Loading

Make scroll offset uncontrollable in favor of ref methods #1320

Make scroll offset uncontrollable in favor of ref methods #1320

Conversation

somi92 commented Feb 5, 2019

wuweiweiwu commented Feb 24, 2019

wuweiweiwu left a comment

Choose a reason for hiding this comment

wuweiweiwu May 3, 2019

Choose a reason for hiding this comment

somi92 May 8, 2019

Choose a reason for hiding this comment

somi92 commented May 4, 2019

wuweiweiwu commented May 6, 2019

codecov-io commented Nov 13, 2019 • edited Loading

Codecov Report

codecov-io commented Nov 13, 2019 •

edited

Loading