Nuke the Viewport component, FCize Canvas #1767
Conversation
| static displayName = 'Canvas'; | ||
|
|
||
| readonly state: Readonly<CanvasState> = { |
There was a problem hiding this comment.
Maybe considering to move this scroll related functions to Grid? Even we use ref.current.setScrollLeft to populate the scrolling offset, we could still split the horizontal scroll bar out of the Canvas (ag-grid, airtable are all doing like this, though google sheet and Microsoft excel online is using <canvas/> for rendering the cells, scroll bar is also splitted). After viewport is removed, I think it will be easier and clearer to see the benefit for splitting the horizontal scroll bar.
bvaughn/react-virtualized#647 (comment)
In Grid.tsx, the horizontal scroll bar only sets the scrollLeft to the Grid, and it can use Header ref and Canvas ref to set scroll left so everything will be in sync. Although we don't need to do the split thing right now.
There was a problem hiding this comment.
I'm not entirely sure I understand what you're proposing, maybe let's discuss this in another PR?
There was a problem hiding this comment.
I was talking about this.
If we move the scrollLeft / scrolLTop state in Grid.tsx level, we don't need to pass the onScroll to Canvas anymore, the state in Grid.tsx would just trigger the Header setScrollLeft call. Rendering the scroll bar separately can make sure header.ref.setScrolLeft and canvas.ref.setScrollLeft being called at same time, so everything will be in sync.
There was a problem hiding this comment.
But we need a component to scroll so we can call the setScrollLeft methods, are you suggesting we add another scroll layer on top of Canvas?
There was a problem hiding this comment.
It's technically not a scroll layer, it's just a div with height: 17px at the bottom of the current viewport (well, you removed it) which only handles the UI horizontal scrolling event. We know the row full width, and displaying with, so the scroll left offset will be same.
nstepien
changed the title
…ome row/cell re-renders
| if (scrollToRowIndex) { | ||
| scrollToRow(scrollToRowIndex); | ||
| } | ||
| }, [scrollToRowIndex]); // eslint-disable-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
I think we can move the scrollToRow logic here and remove eslint-disable-line react-hooks/exhaustive-deps
There was a problem hiding this comment.
If we don't disable the eslint rule, we'll end up scrolling to the row every time we change any other dependency changes. 🤷♂
| if (current) { | ||
| current.scrollTop += this.props.rowHeight + this.getClientScrollTopOffset(current); | ||
| current.scrollTop = (rowIdx + 1) * rowHeight - clientHeight; |
There was a problem hiding this comment.
Do we need to check if rowIdx < rowsCount?
There was a problem hiding this comment.
We could but it's not necessary. If we try to scroll to last row + 1, we'll end up scrolling to the last row anyway.
It also enables this scenario:
- select a cell on the last row
- scroll up, hide last row
- press down arrow
- we scroll back to the last row
| if (!current) return; | ||
| const { rowHeight, rowsCount, height } = this.props; | ||
| current.scrollTop = Math.min( | ||
| scrollToRowIndex * rowHeight, | ||
| rowsCount * rowHeight - height |
There was a problem hiding this comment.
Do we need to use clientHeight instead of height here?
There was a problem hiding this comment.
I've removed this now.
| this.scrollToColumn(idx); | ||
| }; | ||
| function handleHitColummBoundary({ idx }: Position) { | ||
| scrollToColumn(idx, columnMetrics.columns); |
There was a problem hiding this comment.
Is there a reason to pass columnMetrics.columns? Can we not read it directly?
There was a problem hiding this comment.
It simplifies the dependencies for the EventTypes.SCROLL_TO_COLUMN useEffect.
* Replace Canvas placeholders with top/bottom padding
* Move Viewport scroll state and logic into Canvas
* Nuke Viewport component
* Nuke Viewport file
* FCize Canvas
* completely unwrap the props object
* Clean up a few props, fix tests
* Cleanup Canvas getRows
* fix new scrollLeft/Top usage in handleScroll
* Fix rowVisible{Start,End}Idx, improve top/bottom navigation accuracy
* Don't use useMemo to get the clientHeight
* Fix rowOverscanEndIdx usage
* Fix the tests
* Move handleRowSelectionChange back in the root component to prevent some row/cell re-renders
* improve isScrolling example
* Remove viewportWidth props
* Remove scrollToRow, simplify/fix scrollToRowIndex usage
* Add a comment
* Don't disable the eslint rule
No description provided.