Fixing and adding tests for ComboBox onLoadMore called correctly - #2466
Conversation
|
|
||
| expect(onOpenChange).toHaveBeenCalledTimes(1); | ||
| expect(onOpenChange).toHaveBeenCalledWith(true, 'manual'); | ||
| expect(onLoadMore).toHaveBeenCalledTimes(6); |
There was a problem hiding this comment.
@LFDanLu Do you know why this is being called 6 times on initial load in tests? There are three items in the menu. It works in storybook correctly.
I did confirm that without the fix it is called more times in the following test which is the reopen test.
There was a problem hiding this comment.
Pretty odd, I know there are a couple of places loadMore can get called in Virtualizer (onVisibleRectChange, the useLayoutEffect, etc) but can't say for sure without digging into the ScrollView logic.
There was a problem hiding this comment.
Thanks for the info! It sounds like this is okay to proceed forward with.
There was a problem hiding this comment.
you could always see where it's being called from by getting stack traces
let onLoadMore = jest.fn(() => {
try {
console.trace();
} catch (e) {}
});
Which show's @LFDanLu's hunch is correct, those are the two places it's being called from, and in ScrollView it's line 144.
what's interesting is that updateSize is called numerous times. if you place a log on line 143 just before the if console.log(stateRef.current.width, w, stateRef.current.height, h);
0 1000 0 1000
1000 1000 1000 1000
1000 1000 1000 1000
1000 1000 1000 1000
1000 1000 1000 1000
1000 1000 1000 104
1000 1000 104 8
1000 1000 8 8
so it's iterating on the height changing, I'm not familiar enough to know where these sizes are coming from, however, if you change line 255 of the ComboBox test to return 8 for the clientHeight, you can pull down the number of onLoadMore calls to 2
@LFDanLu you're a bit more familiar, do we need to adjust the test for this?
There was a problem hiding this comment.
IMO it would be worthwhile digging into it a little bit. If it becomes too much of a deep dive into VirtualizerState/ScrollView stuff, we can split it out into a separate issue
|
Build successful! 🎉 |
|
Did this get introduced with this change? https://github.com/adobe/react-spectrum/pull/1698/files |
It looks like it did. @LFDanLu? |
|
Yep that is correct. |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| useLayoutEffect(() => { | ||
| if (!isLoading && onLoadMore && !state.isAnimating) { | ||
| if (state.contentSize.height <= state.virtualizer.visibleRect.height) { | ||
| if (state.contentSize.height > 0 && state.contentSize.height <= state.virtualizer.visibleRect.height) { |
There was a problem hiding this comment.
I was initially apprehensive about getting into a state where state.contentSize.height is 0 even after the first load but I think this should be ok for now. I don't imagine there would be a case where the Virtualizer doesn't have a height at all after the initial load (placeholders would exist or minimum top/bottom padding for the scroll view)
|
@ktabors AH, one more thing perhaps. I was doing some extra testing and we'll still call load more upon opening the ComboBox if the number of items doesn't exceed 2x the scroll view height. We could perhaps change so that it accepts aisScrollEvent param as well and havecall onVisibleRectChange with isScrollEvent = true and
with isScrollEvent = false.
Open to other opinions since this will mean we will only cause extra load mores to happen if the user actually scrolls the view whereas we currently will keep trying to loadMore on each open of the combobox until we are 2x the scroll view height. |
5 participants
Closes #2365
✅ Pull Request Checklist:
📝 Test Instructions:
Tests pass
Added onLoadMore action call logging to
server side filtering with useAsyncListstory.🧢 Your Project:
RSP