Add public methods for animation #641

imadha · 2017-04-04T19:18:05Z

This PR adds the following public methods to Grid:

getScrollTop
getScrollLeft
scrollToPosition

The List component also gets, which passes thru to its own Grid:

getScrollTop
scrollToPosition

Grid.getScrollTop and Grid.getScrollLeft are to be used by components that need to know the calculated offsets in pixels for given row and/or column indexes. This is useful for animating.

🔗 See the comment in this Plunker for a use case:

// TODO This works for numeric rowHeights but it's hacky for function props.
// react-virtualized should expose this as a new List/Grid instance method.

Grid.scrollToPosition is intended to be used much like Grid.scrollToRow, so that a component can call it from its Grid instance and scroll to a specific offset. This is also useful for animation, so that a component does not have to reset its own internal state after animating, or forceUpdate in order to force a change in props.

Added tests to illustrate how they would work. Commits are atomic. ⚛️

💬 Original conversation in Slack where this came from: https://react-virtualized.slack.com/archives/C1XUJH7HB/p1490898846575686

- Abstracts calculation of `scrollTop` in `Grid._updateScrollTopForScrollToRow` to new method - Also adds public method `List.getScrollTop`

- Abstracts calculation of `scrollLeft` in `Grid._updateScrollLeftForScrollToColumn` to new method

- Calls `Grid._setScrollPosition`, falls back to 0 pixels - Also adds `List.scrollToPosition`

bvaughn

Firstly, thank you for taking the time to contribute this PR and for writing up your thoughts and tests clearly. That's much appreciated! 😁

I'm a little wary of adding more public instance methods for things that could be controlled by props because it adds some maintenance complexity, but I understand why you might want these to avoid having to explicitly un-set properties after an animation completes.

That being said, I have a few requests for changes. Hope that's ok. 😄

bvaughn · 2017-04-05T04:20:10Z

source/Grid/Grid.js

    this._updateScrollLeftForScrollToColumn = this._updateScrollLeftForScrollToColumn.bind(this)
+    this._getCalculatedScrollTop = this._getCalculatedScrollTop.bind(this)
    this._updateScrollTopForScrollToRow = this._updateScrollTopForScrollToRow.bind(this)
+    this._setScrollPosition = this._setScrollPosition.bind(this)


I don't see why these 3 new functions are bound. They're internal; they're only called from other methods that have the correct context (this). It doesn't seem like this binding is necessary.

The only reason methods like _onScroll and _setScrollingContainerRef are bound this way is that they're passed as properties to React and may not be called with the correct context.

While writing this comment I noticed that _updateScrollLeftForScrollToColumn and _updateScrollTopForScrollToRow no longer need to be bound since they aren't passed directly to updateScrollIndexHelper anymore. 😄

bvaughn · 2017-04-05T04:21:22Z

source/Grid/Grid.js

+   */
+  scrollToPosition ({
+    scrollLeft = 0,
+    scrollTop = 0


I don't think scrollLeft or scrollTop should have default values of 0. That will make them required parameters. The _setScrollPosition helper method allows either param to be omitted, in which case the current value in state won't be modified. This seems convenient if you want to scroll one axis of Grid without affecting the other.

Oh that's a really good point. I was imagining you'd always pass in your x and y here but it's perfectly reasonable to only want to move on one axis.

bvaughn · 2017-04-05T04:24:14Z

source/Grid/Grid.js

+  /**
+   * Gets a calculated value to be used for `scrollLeft`
+   */
+  getScrollLeft (props = this.props, state = this.state) {


What's the reason for these 2 methods to accept props and state as parameters instead of just using the current values (this.props and this.state)? External components really shouldn't know or care about or touch instance.state. And passing in props seems like it would make the method more complicated to use.

I was imagining something more like getScrollLeft(columnIndex: number): number- or maybe even getOffsetForCell({ columnIndex, rowIndex }) => { scrollLeft, scrollTop }.

Were you trying to allow this method to be called during componentWillReceiveProps / componentWillUpdate with a newer version of props than Grid currently had? I'm wary of that- in part because of the disconnect between externally-visible props and internally managed state.

Edit for clarity: It's okay for methods like _getCalculatedScrollLeft to accept params for props and state since they're private/internal methods. I just don't think we should expose this publicly.

They accept props and state mainly because of laziness and not breaking out the parts I think we actually care about. 😅

The main reason I see this being used for is computing a "target" scrollTop (or scrollLeft) based on row or column index. Really, the only things I need to pass are the indexes and the alignment.

getOffsetForCell({ columnIndex, rowIndex, scrollToAlignment }) => { scrollLeft, ScrollTop }

should do the the trick.

bvaughn · 2017-04-05T04:28:45Z

source/List/List.js

  }

+  /** See Grid#getScrollTop */
+  getScrollTop (props = this.props) {


Same comment as above RE Grid ^

- Remove unneeded function bindings - Do not default `Grid.scrollToPosition` params to 0 - Add `Grid.getOffsetForCell` and `List.getOffsetForRow` with more specific params

imadha · 2017-04-05T21:42:14Z

Thanks for taking the time to review my changes @bvaughn! Don't hesitate to let me know if you'd like anything else updated.

On this note:

I'm a little wary of adding more public instance methods for things that could be controlled by props because it adds some maintenance complexity, but I understand why you might want these to avoid having to explicitly un-set properties after an animation completes.

I fully understand this, and the main reason for adding scrollToPosition is exactly what you said, basically avoid unsetting props in the same way one might use scrollToCell. If that's a pattern you'd rather implementers handle, then let's retract that change.

bvaughn

This looks good. I like the fact that it's a pretty small footprint. Thanks for making the changes I requested!

bvaughn · 2017-04-06T02:52:54Z

source/Grid/Grid.jest.js

-        scrollToColumn: 24
+      const { scrollLeft, scrollTop } = grid.getOffsetForCell({
+        columnIndex: 24,
+        rowIndex: 49


Much nicer 👍

bvaughn · 2017-04-06T02:56:05Z

source/Grid/Grid.js

+    scrollToAlignment = this.props.scrollToAlignment
+  } = {}) {
+    const scrollToColumn = columnIndex >= 0 ? columnIndex : this.props.scrollToColumn
+    const scrollToRow = rowIndex >= 0 ? rowIndex : this.props.scrollToRow


Tiny nit: You could set the default columnIndex and rowIndex values in the same way as you do the scrollToAlignment for consistency.

bvaughn · 2017-04-06T03:18:40Z

I made a couple of small tweaks (eg renamed scrollToAlignment parameter to alignment and added a few more tests and docs. Otherwise I'm merging this. Thanks again!

bvaughn · 2017-04-06T03:23:00Z

Releasing as 9.7.0 now. Gave you credit in the changelog. Thanks!

imadha · 2017-04-06T06:14:57Z

bvaughn · 2017-04-06T16:21:44Z

You are welcome!

I look forward to seeing what you build with these new methods. Do share!

imadha added 3 commits April 4, 2017 12:03

Adds public method: Grid.getScrollTop

47f10e4

- Abstracts calculation of `scrollTop` in `Grid._updateScrollTopForScrollToRow` to new method - Also adds public method `List.getScrollTop`

Adds public method: Grid.getScrollLeft

11c78ab

- Abstracts calculation of `scrollLeft` in `Grid._updateScrollLeftForScrollToColumn` to new method

Adds public method: Grid.scrollToPosition

4d73d97

- Calls `Grid._setScrollPosition`, falls back to 0 pixels - Also adds `List.scrollToPosition`

bvaughn requested changes Apr 5, 2017

View reviewed changes

Work in changes requested in PR bvaughn#641

21e791e

- Remove unneeded function bindings - Do not default `Grid.scrollToPosition` params to 0 - Add `Grid.getOffsetForCell` and `List.getOffsetForRow` with more specific params

bvaughn approved these changes Apr 6, 2017

View reviewed changes

bvaughn merged commit 21e791e into bvaughn:master Apr 6, 2017

Uh oh!

Add public methods for animation #641

Add public methods for animation #641

Uh oh!

Conversation

imadha commented Apr 4, 2017

Uh oh!

bvaughn left a comment

Choose a reason for hiding this comment

Uh oh!

bvaughn Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

bvaughn Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

imadha Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

imadha Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

imadha commented Apr 5, 2017

Uh oh!

bvaughn left a comment

Choose a reason for hiding this comment

Uh oh!

bvaughn Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

bvaughn Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Apr 6, 2017

Uh oh!

bvaughn commented Apr 6, 2017

Uh oh!

imadha commented Apr 6, 2017

Uh oh!

bvaughn commented Apr 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

imadha Apr 5, 2017 •

edited

Loading

imadha Apr 5, 2017 •

edited

Loading