FixedSizeList fixes for onItemsRendered & partially-visible items #274
Conversation
|
From a brief look at |
|
I think it's probably not a good idea to merge a change that results in different behavior between the list/grid components. That's not to say you need to fix them all, but it might be a while before I get around to it. I might try to spend some time on it this weekend, but no promises on when I'll be able to finish it 😄 |
| } else if (scrollOffset < minOffset) { | ||
| // For auto alignment, if size isn't an even multiple of itemSize, | ||
| // always put the partial item at the end of the viewport because it | ||
| // looks better than at the beginning. Exception: don't do this if the |
There was a problem hiding this comment.
Not sure I understand the reasoning behind this added complexity. Why does it "look better"?
There was a problem hiding this comment.
I think I disagree with this change. Probably going to remove it. It breaks the specification of "auto" scroll behavior, by over-scrolling in the case when you're scrolling from a lower to higher index.
There was a problem hiding this comment.
Imagine a list with itemSize=30 and size=100. Current behavior when scrolling forward (lower to higher index) with align=auto will look like |--|------|------|--xx--|, where 'xx' is the desired item and each - represents 5px.
I'm suggesting that |------|------|--xx--|--| is a better way to handle the case when items don't fit exactly into the viewport, because people generally read top-to-bottom (or left-to-right for horizontal LTR, or right-to-left for horiz RTL). Users are used to seeing partial items at the end of a list, but seeing them at the start of a list is a somewhat jarring user experience that I was hoping to avoid.
It's something of a judgement call. I don't feel very strongly about this one way or the other, esp. because my current use of react-window doesn't trigger this case. If you take it out I won't be offended. ;-)
BTW, I've been using the relatively-new CSS setting scroll-snap-type and it really improves UX for react-window by enforcing whole-item scrolling, so you're not left with partial items visible.
There was a problem hiding this comment.
Yeah, I think I understood the suggestion just not the reasoning behind it 😄 thanks for elaborating~
BTW, I've been using the relatively-new CSS setting
scroll-snap-typeand it really improves UX for react-window by enforcing whole-item scrolling, so you're not left with partial items visible.
Ah, that's interesting. I haven't played with the two yet. I can see how it might be a UX improvement!
There was a problem hiding this comment.
Yep scroll-snap-type and scroll-snap-align have been big (and very easy) UX wins.
I've also had good results with scroll-behavior: smooth; which makes scrollToItem look really slick as the item slides gracefully into position. The only caveat with this one is that if you have an initial scroll offset that's large, then it smooth-scrolls on mount which can take 500-1000 msecs of items speeding by before it gets to the right item. IMHO this looks bad (and delays initial user interaction) so I've had to add this style using a useEffect after mounting.
It's amazing how much better browsers in general (and CSS in particular) have gotten in the last 5 years. The surface area is huge though-- hard to keep up with!
| // "Centered" offset is usually the average of the min and max | ||
| // offsets. But near the beginning or end of the list, this math | ||
| // doesn't produce the actual closest-to-center offset, so we | ||
| // override. |
|
I'm going to be making some changes in this branch btw. (Just letting you know so that we don't conflict.) |
|
No worries. I wasn't planning to make any changes until I see what cool stuff you're doing. ;-) |
|
BTW, @bvaughn one line of code I didn't fully understand was this one: react-window/src/FixedSizeList.js Line 60 in c71baad
But looking at the context of that code... react-window/src/FixedSizeList.js Lines 56 to 64 in c71baad ... the condition above it ( Mostly I didn't understand the reason for the additional complexity of that comparison, which made me wonder if I was missing something more fundamental. |
Yeah I think you're right that this could be simplified :) |
|
So the good news is, I ported the new |
|
Squashed+merged in 51a24c4. Will be releasing shortly. |
|
Fixes published as 1.8.4 Thanks for your help |
|
Very cool. Thanks @bvaughn! Ironically, after I filed this PR I realized that I needed to know which items are partially vs. fully visible (e.g. to switch selection in one list after another list scrolls.. but not until the new item scrolls fully into view). So I had to switch from Regardless, it was fun to be able to help make react-window better. Good library BTW... very useful. Thanks so much for making it great! |
This PR fixes a few related issues with
FixedSizeList:onItemsRenderedcalls forFixedSizeList. This was done with pen-and-paper and manual calculation to ensure that the existing implementation didn't influence expected results. Most of the changes involved reducingvisibleStopIndexandoverscanStopIndexby one index because of Off-by-one error in onItemsRendered reports visibleStopIndex that's not visible #270.visibleStopIndexis correctly inonItemsRendered. Previously,visibleStopIndexwas one item higher than the last visible index. Fixes Off-by-one error in onItemsRendered reports visibleStopIndex that's not visible #270.onItemsRenderedandscrollToItem.align: 'center'option ofscrollToItemin cases where the viewport is near the beginning or end of the list where the existing method of calculating the centered offset (averagingminOffsetandmaxOffset) didn't produce a correct result.align: 'auto'option ofscrollToItemto handle the case where items don't fit evenly in the viewport. Now, when scrolling forward the partially-visible item will always be shown hanging off the end of the viewport. Exception: at the end of the list, there's no room to show the partial at the end of the viewport so it's shown at the beginning instead. Previously, forward scrolls would hang the partial item off the top of the viewport, which IMHO looked really weird when there wasn't an obvious reason for the visual weirdness (like being at the end of the list). Note that partially-visible items were broken in several ways previously, so I don't think this is a breaking change-- because no one could have used the current implementation successfully withsize % itemSize !== 0. Also note that this change doesn't affect backwards scrolls because the partial (if any) was already shown at the bottom when scrolling up in the current implementation.overscanCountto show a default value of2(which matches react-window's source code). Previously, the default value shown was1which didn't match the code. We could go the other way and change the default in the code to match the docs (which would make the default faster!) but I'd worry about making this change without an explicit breaking change because it would affect user-visible behavior during fast scrolls.getOffsetForIndexAndAlignment, simplified the conditionalscrollOffset - minOffset < maxOffset - scrollOffsettoscrollOffset < minOffsetwhich returns equivalent results in that context but is more readable.In theory there is a breaking change in this PR: fixing
visibleStopIndex/overscanStopIndexmay break clients who are depending on the broken behavior, e.g. if they assume that the*StopIndexvalues were supposed to be exclusive. @bvaughn - I dunno if this is worth a major version bump or not.Note that this PR only fixes
FixedSizeList. Looking at the source forVariableSizeListand the*Gridcomponents, it's likely that they share many of the same issues. In a perfect world I'd have fixed those too, but this PR took more time than I expected so I didn't have time to fix other components. Hopefully someone can use changes in this PR as inspiration for fixing those other components too!