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

overscanIndicesGetter doesnt know if its a horizontal or vertical scroll #627

Closed
offsky opened this issue Mar 23, 2017 · 3 comments
Closed
Assignees

Comments

@offsky
Copy link
Contributor

offsky commented Mar 23, 2017

If you implement overscanIndicesGetter, it gets called twice on every scroll event. Once to get the horizontal value and again to get the vertical value. However, nothing is passed in to let us know which of these it is. If we want to return different values for horizontal vs vertical scrolling, it isn't possible.

This is what the code is

      const overscanColumnIndices = overscanIndicesGetter({
        cellCount: columnCount,
        overscanCellsCount: overscanColumnCount,
        scrollDirection: scrollDirectionHorizontal,
        startIndex: this._renderedColumnStartIndex,
        stopIndex: this._renderedColumnStopIndex
      })

      const overscanRowIndices = overscanIndicesGetter({
        cellCount: rowCount,
        overscanCellsCount: overscanRowCount,
        scrollDirection: scrollDirectionVertical,
        startIndex: this._renderedRowStartIndex,
        stopIndex: this._renderedRowStopIndex
      })

I propose passing another value for horiz/vertical scrolling. Something like this:

      const overscanColumnIndices = overscanIndicesGetter({
        cellCount: columnCount,
        overscanCellsCount: overscanColumnCount,
        scrollDirection: scrollDirectionHorizontal,
        horizontal: true,
        startIndex: this._renderedColumnStartIndex,
        stopIndex: this._renderedColumnStopIndex
      })

      const overscanRowIndices = overscanIndicesGetter({
        cellCount: rowCount,
        overscanCellsCount: overscanRowCount,
        scrollDirection: scrollDirectionVertical,
        horizontal: false,
        startIndex: this._renderedRowStartIndex,
        stopIndex: this._renderedRowStopIndex
      })

What do you think?

@bvaughn
Copy link
Owner

bvaughn commented Mar 23, 2017

Good observation. I probably prefer direction: 'vertical' | 'horizontal' but I don't feel strongly. Since this would be a backwards compatible change, I'd be happy to accept a PR for it.

@offsky
Copy link
Contributor Author

offsky commented Mar 24, 2017

I'll do this. One additional thing to consider. When using a List, the overscanIndexGetter is being called for both Horizontal and Vertical, even though there is no horizontal. Should I also optimize this away so that Lists don't ask the overscanner for horizontal?

@bvaughn
Copy link
Owner

bvaughn commented Mar 24, 2017

Hm...if it doesn't add much complexity to Grid to do this for List and Table then...sure. If it does then I'd say no.

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

No branches or pull requests

2 participants