-
Notifications
You must be signed in to change notification settings - Fork 9
SCC-5375: Multiselect lazy loading #2007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Changes from 10 commits
6b10163
7deaab1
3c7a344
6a1802f
344828f
71b2031
ee5a4dc
0ca1936
d257c93
5da33c4
9babab3
56e243e
5f98c62
0dd92ad
88e8857
831cde6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,56 @@ | ||
| import { axe } from "jest-axe"; | ||
| import { render, screen, waitFor } from "@testing-library/react"; | ||
| import { act, render, screen, waitFor } from "@testing-library/react"; | ||
| import renderer from "react-test-renderer"; | ||
| import userEvent from "@testing-library/user-event"; | ||
| import { useEffect } from "react"; | ||
| import MultiSelect from "./MultiSelect"; | ||
| import useMultiSelect from "../../hooks/useMultiSelect"; | ||
| import withLazyLoadItems from "./MultiSelectWithLazyLoadItems"; | ||
|
|
||
| jest.mock("../../hooks/useSafeId", () => ({ | ||
| ...jest.requireActual("../../hooks/useSafeId"), | ||
| useSafeId: jest.fn((id) => id || "test-id"), | ||
| })); | ||
|
|
||
| let intersectionCallback: IntersectionObserverCallback | null = null; | ||
|
|
||
| const observeMock = jest.fn(); | ||
| const disconnectMock = jest.fn(); | ||
|
|
||
| beforeEach(() => { | ||
| intersectionCallback = null; | ||
| observeMock.mockClear(); | ||
| disconnectMock.mockClear(); | ||
|
|
||
| window.IntersectionObserver = jest.fn().mockImplementation((callback) => { | ||
| intersectionCallback = callback; | ||
| return { | ||
| observe: observeMock, | ||
| disconnect: disconnectMock, | ||
| unobserve: jest.fn(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't also need to be cleared? |
||
| }; | ||
| }); | ||
| }); | ||
|
|
||
| const triggerIntersection = () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice update. How did you catch that the mock scroll behavior was not actually working as expected?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test was failing because no more items were being loaded on scroll - seems like the scroll handler was taking over. I also didn't realize earlier that the IntersectionObserver doesn't work in the test environment because the environment doesn't have the browser layout engine. |
||
| if (!intersectionCallback) return; | ||
|
|
||
| intersectionCallback( | ||
| [ | ||
| { | ||
| isIntersecting: true, | ||
| target: document.createElement("div"), | ||
| intersectionRatio: 1, | ||
| time: 0, | ||
| boundingClientRect: {} as DOMRectReadOnly, | ||
| intersectionRect: {} as DOMRectReadOnly, | ||
| rootBounds: null, | ||
| }, | ||
| ], | ||
| {} as IntersectionObserver | ||
| ); | ||
| }; | ||
|
|
||
| const items = [ | ||
| { id: "dogs", name: "Dogs", isDisabled: false }, | ||
| { id: "cats", name: "Cats", isDisabled: false }, | ||
|
|
@@ -470,6 +510,42 @@ describe("MultiSelect", () => { | |
| ); | ||
| }); | ||
|
|
||
| it("should lazily load more list items while scrolling in lazy load mode", async () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clever test!! also wondering: did you try a more involved mock of the intersection observer and it got too messy ? It seems like it would be difficult to mock
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not 🤔 I could try it though if that would be preferred
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries just wondering
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on the good test and good mock. |
||
| render( | ||
| <MultiSelect | ||
| id="multiselect-lazy-load-id" | ||
| buttonText="Multiselect button text" | ||
| items={withLazyLoadItems} | ||
| isDefaultOpen={true} | ||
| isSearchable={false} | ||
| isBlockElement={false} | ||
| listOverflow="lazy-load" | ||
| selectedItems={selectedTestItems} | ||
| onChange={() => null} | ||
| onClear={() => null} | ||
| /> | ||
| ); | ||
|
|
||
| expect(screen.queryAllByRole("checkbox")).toHaveLength(25); | ||
|
|
||
| await waitFor(() => expect(observeMock).toHaveBeenCalled()); | ||
|
|
||
| act(() => triggerIntersection()); | ||
| await waitFor(() => | ||
| expect(screen.queryAllByRole("checkbox")).toHaveLength(50) | ||
| ); | ||
|
|
||
| act(() => triggerIntersection()); | ||
| await waitFor(() => | ||
| expect(screen.queryAllByRole("checkbox")).toHaveLength(80) | ||
| ); | ||
|
|
||
| act(() => triggerIntersection()); | ||
| await waitFor(() => | ||
| expect(screen.queryAllByRole("checkbox")).toHaveLength(116) | ||
| ); | ||
| }); | ||
|
|
||
| it("should call onChange when an item without child items or a child item is selected/unselected", () => { | ||
| const onChangeMock = jest.fn(); | ||
| const onMixedStateChangeMock = jest.fn(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,13 @@ import { | |
| ChakraComponent, | ||
| useMultiStyleConfig, | ||
| } from "@chakra-ui/react"; | ||
| import React, { forwardRef, useEffect, useRef, useState } from "react"; | ||
| import React, { | ||
| forwardRef, | ||
| useCallback, | ||
| useEffect, | ||
| useRef, | ||
| useState, | ||
| } from "react"; | ||
|
|
||
| import Accordion from "./../Accordion/Accordion"; | ||
| import Button from "./../Button/Button"; | ||
|
|
@@ -24,7 +30,11 @@ export interface MultiSelectItem { | |
| } | ||
| export const multiSelectWidthsArray = ["fitContent", "full"] as const; | ||
| export type MultiSelectWidths = typeof multiSelectWidthsArray[number]; | ||
| export const multiSelectListOverflowArray = ["scroll", "expand"] as const; | ||
| export const multiSelectListOverflowArray = [ | ||
| "scroll", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interested in marty/edwin thoughts but maybe lazy-load should be a flag you can set if you've picked
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah that does make sense
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me review before you make this update. Without having looked too closely at the PR, Emma's comment sounds reasonable. Perhaps it can be an independent flag that works for both
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this shouldn't be possible on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be wrong to always have lazy load be enabled for the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, agreed. Here's another idea. Although, I am not sure if the timing of this is possible. Can lazy load be automatically enabled if the total options is greater than Do we know the threshold when performance takes a hit?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about the threshold. I'm open to logging performance/render times compared to the number of items, but I imagine that performance could also vary based on how it's used in the consuming app? Right now
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let's keep it at |
||
| "expand", | ||
| "lazy-load", | ||
| ] as const; | ||
| export type MultiSelectListOverflowTypes = | ||
| typeof multiSelectListOverflowArray[number]; | ||
| export interface SelectedItems { | ||
|
|
@@ -51,7 +61,7 @@ export interface MultiSelectProps extends BoxProps { | |
| /** The items to be rendered in the Multiselect as checkbox options. */ | ||
| items: MultiSelectItem[]; | ||
| /** listOverflow is a property indicating how the list should handle overflow, | ||
| * with options limited to either "scroll" or "expand." */ | ||
| * with options limited to "scroll", "expand", or "lazy-load". */ | ||
| listOverflow?: MultiSelectListOverflowTypes; | ||
| /** The action to perform for the clear/reset button of individual MultiSelects. */ | ||
| onClear?: () => void; | ||
|
|
@@ -111,6 +121,13 @@ export const MultiSelect: ChakraComponent< | |
| const expandToggleButtonRef: React.RefObject<HTMLButtonElement> = | ||
| useRef<HTMLButtonElement>(); | ||
|
|
||
| // Used for Intersection Observer for lazy loading | ||
| const itemsListRef: React.RefObject<HTMLDivElement> = | ||
| useRef<HTMLDivElement>(); | ||
| // Observation target for Intersection Observer | ||
| const lazyLoadTargetRef: React.RefObject<HTMLDivElement> = | ||
| useRef<HTMLDivElement>(); | ||
|
|
||
| // Tells `Accordion` to close if open when user clicks outside of the container | ||
| const handleClickOutside = (e) => { | ||
| if (e.type === "mousedown") { | ||
|
|
@@ -162,12 +179,26 @@ export const MultiSelect: ChakraComponent< | |
|
|
||
| const isOverflowExpand = | ||
| items.length > defaultItemsVisible && listOverflow === "expand"; | ||
| const lazyLoadIncrementNum = 20; | ||
| const isOverflowLazy = | ||
| items.length > defaultItemsVisible + lazyLoadIncrementNum && | ||
| listOverflow === "lazy-load"; | ||
| const defaultItemsList = React.useMemo( | ||
| () => (isOverflowExpand ? items.slice(0, defaultItemsVisible) : items), | ||
| [isOverflowExpand, items, defaultItemsVisible] | ||
| ); | ||
| const [itemsList, setItemsList] = useState(defaultItemsList); | ||
| const [isExpandable, setIsExpandable] = useState(true); | ||
| const [lazyItemsVisible, setLazyItemsVisible] = useState( | ||
| defaultItemsVisible + lazyLoadIncrementNum | ||
| ); | ||
|
|
||
| const hasScrollablePanel = | ||
| listOverflow === "scroll" || listOverflow === "lazy-load"; | ||
|
|
||
| const visibleItemsList = isOverflowLazy | ||
| ? itemsList.slice(0, lazyItemsVisible) | ||
| : itemsList; | ||
|
|
||
| const selectedItemsCount: number = | ||
| selectedItems[mainId]?.items.length || 0; | ||
|
|
@@ -245,6 +276,19 @@ export const MultiSelect: ChakraComponent< | |
| return <Box>No options found</Box>; | ||
| }; | ||
|
|
||
| const loadMoreLazyItems = useCallback(() => { | ||
| if (!isOverflowLazy) { | ||
| return; | ||
| } | ||
|
|
||
| setLazyItemsVisible((previousVisibleItems) => | ||
| Math.min( | ||
| previousVisibleItems * 1.2 + lazyLoadIncrementNum, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you create a |
||
| itemsList.length | ||
| ) | ||
| ); | ||
| }, [isOverflowLazy, itemsList]); | ||
|
|
||
| const onChangeSearch = (event) => { | ||
| const value = event.target.value.trim().toLowerCase(); | ||
| if (!value) { | ||
|
|
@@ -291,6 +335,33 @@ export const MultiSelect: ChakraComponent< | |
| setItemsList(isExpandable ? defaultItemsList : items); | ||
| }, [isExpandable, defaultItemsList, items]); | ||
|
|
||
| React.useEffect(() => { | ||
| if ( | ||
| !isOverflowLazy || | ||
| !itemsListRef.current || | ||
| !lazyLoadTargetRef.current | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const observer = new IntersectionObserver( | ||
| (entries) => { | ||
| if (entries.some((entry) => entry.isIntersecting)) { | ||
| loadMoreLazyItems(); | ||
| } | ||
| }, | ||
| { | ||
| root: itemsListRef.current, | ||
| threshold: 0, | ||
| rootMargin: `${12 * lazyItemsVisible}px`, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also create another constant var for |
||
| } | ||
| ); | ||
|
|
||
| observer.observe(lazyLoadTargetRef.current); | ||
|
|
||
| return () => observer.disconnect(); | ||
| }, [isOverflowLazy, loadMoreLazyItems, lazyItemsVisible]); | ||
|
|
||
| const ExpandToggleButton = (): JSX.Element => { | ||
| return ( | ||
| <Button | ||
|
|
@@ -402,22 +473,24 @@ export const MultiSelect: ChakraComponent< | |
|
|
||
| const accordionPanel = ( | ||
| <Box position="relative"> | ||
| {isSearchable && !isOverflowExpand ? ( | ||
| {isSearchable && hasScrollablePanel ? ( | ||
| <Box position="sticky" top="0" marginBottom="12px" zIndex="1"> | ||
| {searchInput} | ||
| </Box> | ||
| ) : isSearchable && isOverflowExpand ? ( | ||
| ) : isSearchable && (isOverflowExpand || isOverflowLazy) ? ( | ||
| searchInput | ||
| ) : null} | ||
|
|
||
| <Box | ||
| data-testid={isOverflowLazy ? `${mainId}-items-list` : undefined} | ||
| ref={itemsListRef} | ||
| maxHeight={listHeight} | ||
| overflowY="auto" | ||
| paddingTop="xxs" | ||
| paddingLeft="xs" | ||
| paddingBottom="xxs" | ||
| > | ||
| {itemsList.length === 0 ? ( | ||
| {visibleItemsList.length === 0 ? ( | ||
| <NoSearchResults /> | ||
| ) : ( | ||
| <> | ||
|
|
@@ -430,13 +503,17 @@ export const MultiSelect: ChakraComponent< | |
| showLabel={false} | ||
| name="multi-select-checkbox-group" | ||
| > | ||
| {itemsList.map((item: MultiSelectItem) => ( | ||
| {visibleItemsList.map((item: MultiSelectItem) => ( | ||
| <React.Fragment key={item.id}> | ||
| {getMultiSelectCheckboxItem(item)} | ||
| </React.Fragment> | ||
| ))} | ||
| </CheckboxGroup> | ||
| {isOverflowExpand && <ExpandToggleButton />} | ||
| {isOverflowLazy && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense why we need this, but can you add a short comment describing why? When we revisit this component in the future, it'll be clear why this is needed in the DOM. |
||
| visibleItemsList.length < itemsList.length && ( | ||
| <Box ref={lazyLoadTargetRef} height="1px" /> | ||
| )} | ||
| </> | ||
| )} | ||
| </Box> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: update "with" to "when"?