Fix issue with empty space in table if row heights change (#510) #511
Conversation
This can happen if you scroll to the last row of the Expanded rows example. Then expand and collapse the last row.
wcjordan
requested review from
mycrobe,
novakps,
somavara and
pradeepnschrodinger
src/reducers/computeRenderedRows.js
Outdated
| while (rowIdx >= 0 && totalHeight < maxAvailableHeight) { | ||
| totalHeight += updateRowHeight(state, rowIdx); | ||
| startIdx = rowIdx; | ||
| rowIdx += reverseStep; |
There was a problem hiding this comment.
Unless there's intention to take bigger steps in the future, can we remove reverseStep, and then --rowIdx; here?
There was a problem hiding this comment.
Yep, that's fine. Will do
src/reducers/computeRenderedRows.js
Outdated
| @@ -145,6 +145,23 @@ function calculateRenderedRowRange(state, scrollAnchor) { | |||
| rowIdx += step; | |||
| } | |||
|
|
|||
| /* Handle the case where we rows have shrunk and we don't have enough content | |||
| // Calculate offset needed to position last row at bottom of viewport | ||
| // This should be negative and represent how far the first row needs to be offscreen | ||
| firstOffset = Math.min(availableHeight - totalHeight, 0); | ||
| firstOffset = firstOffset + Math.min(availableHeight - totalHeight, 0); |
There was a problem hiding this comment.
It's not clear to me that this change won't affect current behavior when lastIndex !== undefined. Can we guarantee that firstOffset === 0 in that case? Otherwise the offset will be different here.
There was a problem hiding this comment.
Ah ya, that's a bit confusing.
The offset is going to always be 0 when the lastIndex is set. That's because the last index is used to say, "scroll this row into view as the last row of the table." Whenever we do that we're putting that row flush so it's fully in view and not offset. You can verify this by looking at all the cases in scrollAnchor.js.
I'll add a comment here which captures something like this.
There was a problem hiding this comment.
comment added:
// NOTE (jordan): The first offset should always be 0 when lastIndex is defined
// since we don't currently support scrolling the last row into view with an offset.
|
@pradeepnschrodinger when you get a chance, could you give this a review? Thanks |
| In this case process earlier rows as needed and act as if we've scrolled to the last row. | ||
| */ | ||
| let forceScrollToLastRow = false; | ||
| if (totalHeight < maxAvailableHeight && rowIdx === rowsCount && lastIndex === undefined) { |
There was a problem hiding this comment.
why is the lastIndex undefined check required?
There was a problem hiding this comment.
if we're anchoring to the lastIndex then we've already been stepping from the last index row upwards towards the top of the viewport. If we didn't find enough row content, then stepping upwards more won't help.
A follow up question would be whether we need to step down to rows below the last index in that case. I don't think so, but if you can come up with a case to try, I'm happy to test it out.
|
We're working to fix some issues introduced when moving to React's new lifecycle methods. Once those are resolved, we'll release v1.1. |
|
This is now released w/ v1.1. |
Description
Fix issue with empty space in table if row heights change
This can happen if you scroll to the last row of the Expanded rows
example. Then expand and collapse the last row.
Primary: @pradeepnschrodinger
Motivation and Context
Fixes issue #510
How Has This Been Tested?
Tested on the Expanded Rows example, both w/ 2000 rows and w/ 3 rows
Types of changes
Checklist: