Windowing for Select & SelectMulti #870
Conversation
| @@ -42,6 +42,9 @@ const observeMock = function(cb, config) { | |||
| const globalAny = global | |||
| globalAny.IntersectionObserver = observeMock | |||
|
|
|||
| // js-dom doesn't do scrollIntoView | |||
| Element.prototype.scrollIntoView = jest.fn() | |||
There was a problem hiding this comment.
Got an error on this guy in one of the new tests. jsdom/jsdom#1695
| if (contentContainer) { | ||
| setListClientRectOnce(contentContainer) | ||
| setListScrollPosition && | ||
| setListScrollPosition(contentContainer.scrollTop) | ||
| } | ||
| } | ||
| }, 50) |
There was a problem hiding this comment.
Taking any performance boost I can get.
| contentContainer.addEventListener('scroll', scrollListener) | ||
| scrollListener() |
There was a problem hiding this comment.
We now need the height and scroll position right away on initial render.
|
|
||
| return () => { | ||
| contentContainer && | ||
| contentContainer.removeEventListener('scroll', scrollListener) | ||
| setListScrollPosition && setListScrollPosition(0) | ||
| setListClientRect && setListClientRect(undefined) |
There was a problem hiding this comment.
Since these values are going into the context, we have to clear them when the list unmounts, because the provider is still mounted and otherwise persists those previous values.
| if (optionsRefCurrent) { | ||
| const windowedOptions = | ||
| windowedOptionsPropRef && windowedOptionsPropRef.current | ||
| if (optionsRefCurrent && !windowedOptions) { |
There was a problem hiding this comment.
This hook is for letting each option "register" itself to the context when it mounts to support keyboard navigation, but since any option outside the scroll window will not be mounted, we disable this behavior here and add it a different way in useWindowedOptions.
| @@ -80,7 +81,7 @@ export function useKeyDown() { | |||
| } | |||
| } | |||
|
|
|||
| return function handleKeyDown(event: KeyboardEvent<HTMLDivElement>) { | |||
| return throttle(function handleKeyDown(event: KeyboardEvent<HTMLDivElement>) { | |||
There was a problem hiding this comment.
Helps performance when the user clicks and holds an arrow key.
| @@ -62,10 +67,11 @@ export function useOptionEvents< | |||
| } | |||
| } | |||
|
|
|||
| function handleMouseEnter() { | |||
| const handleMouseEnter = throttle(() => { | |||
| if (isScrollingRef && isScrollingRef.current) return | |||
There was a problem hiding this comment.
Solves an existing issue – without this, if you keyboard navigate down to options below the scroll window, causing a small auto-scroll, and your mouse is resting somewhere on the list, a mouseenter will be triggered where your mouse is.
(isScrollingRef set/unset below in useOptionsScroll before scrollIntoView is called.)
| key="select-multi" | ||
| />, | ||
| ], | ||
| ])('custom label text (%s)', (_, jsx) => { |
There was a problem hiding this comment.
Git diff is being funny here. This was an existing test, but the 2 above are now.
| const renderMultiOption = ( | ||
| option: SelectOptionObject, | ||
| index: number, | ||
| scrollIntoView?: boolean |
There was a problem hiding this comment.
Enables scrolling to the bottom of the list if the user arrows up from the first option or to the top of the list if the user arrows down from the last option.
| return false | ||
| } | ||
| return true | ||
| }, [options, propsWindowedOptions]) |
There was a problem hiding this comment.
When to window the options – option length is 100 or more, by default, with prop override.
| scrollToLast, | ||
| } = useWindowedOptions(windowedOptions, options, isMulti) | ||
|
|
||
| const optionsToRender = options && options.slice(start, end + 1) |
There was a problem hiding this comment.
Will include the entire list if windowedOptions is false (see getWindowedListBoundaries.ts).
| @@ -192,25 +252,41 @@ export function SelectOptions({ | |||
|
|
|||
| return ( | |||
| <> | |||
| {options && options.length > 0 | |||
| {options && scrollToFirst | |||
| ? renderToUse(options[0] as SelectOptionObject, 0, true) | |||
There was a problem hiding this comment.
If the user arrows down from the last option.
| options[options.length - 1] as SelectOptionObject, | ||
| 0, | ||
| true | ||
| ) |
There was a problem hiding this comment.
If the user arrows up from the first option.
| 'The `windowedOptions` prop does not support grouped options.' | ||
| ) | ||
| } | ||
| return false |
There was a problem hiding this comment.
Not able to window with grouped options at this time.
| * list of options in order for keyboard navigation to work properly | ||
| * @default false | ||
| */ | ||
| windowedOptions?: boolean |
There was a problem hiding this comment.
I wonder if lazyRenderOptions better describe the behavior? 🤔
There was a problem hiding this comment.
I think convention calls for either "virtualize" or "window" https://reactjs.org/docs/optimizing-performance.html#virtualize-long-lists
I went for "window" because it's shorter but maybe "virtualize" is a more specific word... thoughts? I wonder if @lukelooker has an opinion... 😁
| } = useContext(context) | ||
| /* scroll menu list to specified element on mount */ | ||
| const [newTriggerElement, callbackRef] = useCallbackRef() | ||
| useEffect(() => { | ||
| if (scrollIntoView) { | ||
| if (newTriggerElement) { | ||
| // if (isScrollingRef) isScrollingRef.current = true |
| if (isScrollingRef) isScrollingRef.current = true | ||
| window.requestAnimationFrame(() => { | ||
| if (isScrollingRef) isScrollingRef.current = false | ||
| }) |
| // Resolves "act" warning | ||
| await wait() | ||
|
|
||
| // Close popover to silence act() warning |
| expect(queryByText('0')).toBeInTheDocument() | ||
| expect(queryByText('99')).not.toBeInTheDocument() | ||
|
|
There was a problem hiding this comment.
this doesn't seem to validate that 99 options all render. should you also add that queryByText('99') is in the document?
There was a problem hiding this comment.
Also I'm not sure that I can see any difference between the component rendered here and in the previous test. Is the windowedOptions prop toggled somewhere?
There was a problem hiding this comment.
Oh whoops, good catch – bad copy/pasting on my part. 🤪Now it's checking that "98" (the 99th option) is rendered.
The default behavior is to turn on windowing at 100, which I've read is where performance starts to go downhill. Then the windowedOptions prop can be used to override the default (either on or off, depending on the length of options).
| // If the user keyboard navigates "down" from the last option or "up" from the first option | ||
| // we need to render the top or bottom of the list (which are outside our "window") and scroll there |
There was a problem hiding this comment.
This doesn't seem to be working as expected. I'm continually pressing up arrow in this screencast:
There was a problem hiding this comment.
Oops, looking into it now.
There was a problem hiding this comment.
This should be fixed now. I had moved the logic that automatically turns on windowing at 100 options to the wrong spot. Now it's a hook under Select/utils so it can be used by both Select and SelectMulti.
|
Ready for another round of review @dbchristopher 🙂 |
There was a problem hiding this comment.
This looks really great @mdodgelooker! So awesome this can support such insane use cases now. Everything works really well. Ship it!
✨ Changes
windowedOptionsprop overrides the above (eithertruefor < 100 orfalsefor >= 100)Comboboxbehaviors including keyboard navigating through options via the arrow keys and scrolling to the current value on openSelect/useWindowedOptions.tsxmanages these behaviorsutils/getWindowedListBoundaries.tsis generic and may be used for windowing in other list components✅ Requirements
📷 Screenshots
With windowing:
Without windowing: