-
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 8 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,5 +1,5 @@ | ||
| import { axe } from "jest-axe"; | ||
| import { render, screen, waitFor } from "@testing-library/react"; | ||
| import { fireEvent, render, screen, waitFor } from "@testing-library/react"; | ||
|
EdwinGuzman marked this conversation as resolved.
Outdated
|
||
| import renderer from "react-test-renderer"; | ||
| import userEvent from "@testing-library/user-event"; | ||
| import { useEffect } from "react"; | ||
|
|
@@ -11,6 +11,14 @@ jest.mock("../../hooks/useSafeId", () => ({ | |
| useSafeId: jest.fn((id) => id || "test-id"), | ||
| })); | ||
|
|
||
| const intersectionObserverMock = () => ({ | ||
| observe: () => null, | ||
| disconnect: () => null, | ||
| }); | ||
| window.IntersectionObserver = jest | ||
|
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. Emma already asked if you tested the rc npm release in Research Catalog. Did you also run tests in RC? I noticed that whenever we mock global APIs in the DS, unit tests in consuming apps need to be updated as well. It's not bad, but we would just have to let teams know about the dependency.
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. Yes, the
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. Not a big factor but just something to call out when devs import the release version. Did you have to add it in a global pre-test jest file in RC?
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. Seems like it's only needed if the number of items in the MultiSelect > either in the test files with the MultiSelect, or the global pre-test file is needed. |
||
| .fn() | ||
| .mockImplementation(intersectionObserverMock); | ||
|
|
||
| const items = [ | ||
| { id: "dogs", name: "Dogs", isDisabled: false }, | ||
| { id: "cats", name: "Cats", isDisabled: false }, | ||
|
|
@@ -65,6 +73,22 @@ const itemsWithCount = [ | |
|
|
||
| const defaultItemsVisible = 5; | ||
|
|
||
| const lazyLoadItems = [ | ||
| { id: "item-1", name: "Item 1" }, | ||
| { id: "item-2", name: "Item 2" }, | ||
| { id: "item-3", name: "Item 3" }, | ||
| { id: "item-4", name: "Item 4" }, | ||
| { id: "item-5", name: "Item 5" }, | ||
| { id: "item-6", name: "Item 6" }, | ||
| { id: "item-7", name: "Item 7" }, | ||
| { id: "item-8", name: "Item 8" }, | ||
| { id: "item-9", name: "Item 9" }, | ||
| { id: "item-10", name: "Item 10" }, | ||
| { id: "item-11", name: "Item 11" }, | ||
| { id: "item-12", name: "Item 12" }, | ||
| { id: "item-13", name: "Item 13" }, | ||
| ]; | ||
|
|
||
| const MultiSelectTestComponent = ({ | ||
| multiSelectId, | ||
| initialSelectedItems = {}, | ||
|
|
@@ -470,6 +494,58 @@ 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" | ||
| defaultItemsVisible={3} | ||
| items={lazyLoadItems} | ||
| isDefaultOpen={true} | ||
| isSearchable={false} | ||
| isBlockElement={false} | ||
| listOverflow="lazy-load" | ||
| selectedItems={selectedTestItems} | ||
| onChange={() => null} | ||
| onClear={() => null} | ||
| /> | ||
| ); | ||
|
|
||
| expect(screen.queryAllByRole("checkbox")).toHaveLength(3); | ||
|
|
||
| const listContainer = screen.getByTestId( | ||
| "multiselect-lazy-load-id-items-list" | ||
| ); | ||
| Object.defineProperty(listContainer, "clientHeight", { | ||
| configurable: true, | ||
| value: 500, | ||
| }); | ||
| Object.defineProperty(listContainer, "scrollHeight", { | ||
| configurable: true, | ||
| value: 1000, | ||
| }); | ||
| Object.defineProperty(listContainer, "scrollTop", { | ||
| configurable: true, | ||
| value: 500, | ||
| writable: true, | ||
| }); | ||
|
|
||
| fireEvent.scroll(listContainer); | ||
| await waitFor(() => | ||
| expect(screen.queryAllByRole("checkbox")).toHaveLength(7) | ||
| ); | ||
|
|
||
| fireEvent.scroll(listContainer); | ||
| await waitFor(() => | ||
| expect(screen.queryAllByRole("checkbox")).toHaveLength(12) | ||
| ); | ||
|
|
||
| fireEvent.scroll(listContainer); | ||
| await waitFor(() => | ||
| expect(screen.queryAllByRole("checkbox")).toHaveLength(13) | ||
| ); | ||
| }); | ||
|
|
||
| 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") { | ||
|
|
@@ -153,21 +170,31 @@ export const MultiSelect: ChakraComponent< | |
|
|
||
| const MINIMUM_ITEMS_LIST_HEIGHT = "215px"; | ||
| const MAXIMUM_ITEMS_LIST_HEIGHT = "270px"; | ||
| const listHeight = | ||
| listOverflow === "expand" | ||
| ? "unset" | ||
| : isSearchable | ||
| ? MAXIMUM_ITEMS_LIST_HEIGHT | ||
| : MINIMUM_ITEMS_LIST_HEIGHT; | ||
|
|
||
| const isOverflowExpand = | ||
| items.length > defaultItemsVisible && listOverflow === "expand"; | ||
| const isOverflowLazy = | ||
| items.length > defaultItemsVisible && 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); | ||
|
|
||
| const hasScrollablePanel = listOverflow === "scroll" || isOverflowLazy; | ||
|
|
||
| const listHeight = | ||
| listOverflow === "expand" | ||
| ? "unset" | ||
| : isSearchable | ||
| ? MAXIMUM_ITEMS_LIST_HEIGHT | ||
| : MINIMUM_ITEMS_LIST_HEIGHT; | ||
|
|
||
| const visibleItemsList = isOverflowLazy | ||
| ? itemsList.slice(0, lazyItemsVisible) | ||
| : itemsList; | ||
|
|
||
| const selectedItemsCount: number = | ||
| selectedItems[mainId]?.items.length || 0; | ||
|
|
@@ -245,6 +272,21 @@ export const MultiSelect: ChakraComponent< | |
| return <Box>No options found</Box>; | ||
| }; | ||
|
|
||
| const loadMoreLazyItems = useCallback(() => { | ||
| if (!isOverflowLazy) { | ||
| return; | ||
| } | ||
|
|
||
| setLazyItemsVisible((previousVisibleItems) => | ||
| Math.min( | ||
| previousVisibleItems + | ||
| defaultItemsVisible + | ||
| previousVisibleItems / defaultItemsVisible, // Scales with further scrolling | ||
|
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. What are the benefits to scaling the batch?
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. When I was experimenting with scrolling quickly, the scrolling speed can increase as I scroll further down. The scaling was to prevent too many items from loading at the beginning, while preventing the stuttering that I mentioned above if the user is scrolling quickly to the bottom (which I saw more on research catalog than storybook). Maybe I could set a constant higher amount to load, though @bigfishdesign13 suggested continuing with the scaling, which I can try adapting to the IntersectionObserver.
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. The current configuration works well. No matter how fast I try to scroll, I never hit the bottom. I would prefer to load a constant number of options during the scrolling, but I also don't want users to hit the bottom. The data for all of the options is always available, right? No API call to get more data from the server. So any lag that happens as the options are added is due to rendering, which could be impacted by the power of a users computer or device. If there are concerns about the scaling, let's try a static number and try to find the sweet spot.
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 realized that a lot of the slowness issues I was having without scaling was because I was testing in research catalog dev mode earlier, my mistake. I removed the scaling and set the rootMargin to 300px which seems to work well enough, let me know if there are any issues with that |
||
| itemsList.length | ||
| ) | ||
| ); | ||
| }, [defaultItemsVisible, isOverflowLazy, itemsList]); | ||
|
|
||
| const onChangeSearch = (event) => { | ||
| const value = event.target.value.trim().toLowerCase(); | ||
| if (!value) { | ||
|
|
@@ -287,10 +329,53 @@ export const MultiSelect: ChakraComponent< | |
| }, 1); // Ensure focus logic runs after state update | ||
| }; | ||
|
|
||
| const onItemsListScroll = () => { | ||
|
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. I think this chunk can be replaced by setting
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. This chunk allows for a more gradual and scaled loading - without it, scrolling down just a bit loads a lot of items (even with
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 i see what you're saying here! I think it's worth the workaround to have a better scrolling experience
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, the scrolling-loading could use some polishing. I'd love to have a chat to learn more about the API and the config.
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.
I put these console.logs right before the The scroll handler keeps moving the sentinel away before the observer can reach it. So the observer fires only once, while the scroll handler takes over and it's running a lot. I tried removing Also with regards to, "scrolling down just a bit loads a lot of items" -- isn't the number of items related to the setState in
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. @oliviawongnyc I didn't realize that, thanks for pointing that out! When I was testing in research catalog using just the observer, scrolling fast would cause stuttering. After scrolling all the way to the bottom, it took some time to load more items, and by then the user would be scrolling past the panel entirely. So I think I overcorrected for that and messed up the observer in the process without realizing it. As for the difference, I misinterpreted the comment and thought the chunk was referring to the scaling part of it. Removing the scroll handler changes the behavior slightly but doesn't make the difference I mentioned, sorry for the confusion. I'll take a look at reducing stuttering while only using the IntersectionObserver.
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 removed the scroll handler and tweaked some of the scaling. I tried calculating 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. Changes looking good to me! As for the scaling, what happens with a fixed root margin? Same question about the batching. Does the increment size need to scale? |
||
| if (!isOverflowLazy || !itemsListRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| const { scrollTop, clientHeight, scrollHeight } = itemsListRef.current; | ||
| const scrollThreshold = scrollHeight / 2; // Scales with further scrolling | ||
| const isAtBottom = | ||
|
EdwinGuzman marked this conversation as resolved.
Outdated
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. What do you think about
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. Scratch this. After looking at this longer, I don't think we should use two triggers for the loading, and think we should use the observer over this onScroll. |
||
| scrollTop + clientHeight >= scrollHeight - scrollThreshold; | ||
|
|
||
| if (!isAtBottom) { | ||
| return; | ||
| } | ||
|
|
||
| loadMoreLazyItems(); | ||
| }; | ||
|
|
||
| React.useEffect(() => { | ||
| 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, | ||
| } | ||
| ); | ||
|
|
||
| observer.observe(lazyLoadTargetRef.current); | ||
|
|
||
| return () => observer.disconnect(); | ||
| }, [isOverflowLazy, loadMoreLazyItems]); | ||
|
|
||
| const ExpandToggleButton = (): JSX.Element => { | ||
| return ( | ||
| <Button | ||
|
|
@@ -402,22 +487,25 @@ 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} | ||
| onScroll={isOverflowLazy ? onItemsListScroll : undefined} | ||
| maxHeight={listHeight} | ||
| overflowY="auto" | ||
| paddingTop="xxs" | ||
| paddingLeft="xs" | ||
| paddingBottom="xxs" | ||
| > | ||
| {itemsList.length === 0 ? ( | ||
| {visibleItemsList.length === 0 ? ( | ||
| <NoSearchResults /> | ||
| ) : ( | ||
| <> | ||
|
|
@@ -430,13 +518,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"?