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

Make scroll offset uncontrollable in favor of ref methods #1461

Closed
wants to merge 3 commits into from

Conversation

wuweiweiwu
Copy link
Contributor

@wuweiweiwu wuweiweiwu commented Nov 13, 2019

Changes based on discussion from #999.

Follow up of #1320

  • scrollToCell
  • clean up Grid prop mirroring
  • clean up gDSFP implementations
  • clean up APIs

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.)

* Make scroll offset uncontrollable in favor of ref methods

* 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

* Update MultiGrid.js

* Update ScrollSync.js

* Update ScrollSync.js
@codecov-io
Copy link

codecov-io commented Nov 13, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1461      +/-   ##
==========================================
- 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 489794a...26d286b. Read the comment docs.

@somi92
Copy link

somi92 commented Nov 13, 2019

@wuweiweiwu Glad to see progress on this! Let me know if I can help.

@wuweiweiwu
Copy link
Contributor Author

@somi92 this is thanks to all your work!!

@github-actions github-actions bot added the Stale label Sep 16, 2024
@github-actions github-actions bot closed this Sep 16, 2024
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 95.23810% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.44%. Comparing base (489794a) to head (26d286b).
Report is 51 commits behind head on master.

Files with missing lines Patch % Lines
source/MultiGrid/MultiGrid.js 90.47% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1461      +/-   ##
==========================================
- Coverage   94.70%   94.44%   -0.27%     
==========================================
  Files          47       47              
  Lines        1795     1800       +5     
==========================================
  Hits         1700     1700              
- Misses         95      100       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants