Skip to content
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

feat(ActionList): add Virtualization #2471

Merged
merged 9 commits into from
Jan 15, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/blade/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@
"@mantine/core": "6.0.21",
"@mantine/dates": "6.0.21",
"@mantine/hooks": "6.0.21",
"dayjs": "1.11.10"
"dayjs": "1.11.10",
"react-window": "1.8.6"
saurabhdaware marked this conversation as resolved.
Show resolved Hide resolved
},
"devDependencies": {
"http-server": "14.1.1",
Expand Down Expand Up @@ -222,6 +223,7 @@
"@types/styled-components-react-native": "5.1.3",
"@types/tinycolor2": "1.4.3",
"@types/react-router-dom": "5.3.3",
"@types/react-window": "1.8.8",
"@types/storybook-react-router": "1.0.5",
"any-leaf": "1.2.2",
"args-parser": "1.3.0",
Expand Down
14 changes: 10 additions & 4 deletions packages/blade/src/components/ActionList/ActionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import React from 'react';
import { getActionListContainerRole, getActionListItemWrapperRole } from './getA11yRoles';
import { getActionListProperties } from './actionListUtils';
import { ActionListBox } from './ActionListBox';
import { ActionListBox as ActionListNormalBox, ActionListVirtualizedBox } from './ActionListBox';
import { componentIds } from './componentIds';
import { ActionListNoResults } from './ActionListNoResults';
import { useDropdown } from '~components/Dropdown/useDropdown';
Expand All @@ -17,10 +17,16 @@ import { makeAnalyticsAttribute } from '~utils/makeAnalyticsAttribute';

type ActionListProps = {
children: React.ReactNode[];
isVirtualized?: boolean;
} & TestID &
DataAnalyticsAttribute;

const _ActionList = ({ children, testID, ...rest }: ActionListProps): React.ReactElement => {
const _ActionList = ({
children,
testID,
isVirtualized,
...rest
}: ActionListProps): React.ReactElement => {
const {
setOptions,
actionListItemRef,
Expand All @@ -31,15 +37,15 @@ const _ActionList = ({ children, testID, ...rest }: ActionListProps): React.Reac
filteredValues,
} = useDropdown();

const ActionListBox = isVirtualized ? ActionListVirtualizedBox : ActionListNormalBox;

const { isInBottomSheet } = useBottomSheetContext();

const { sectionData, childrenWithId, actionListOptions } = React.useMemo(
() => getActionListProperties(children),
[children],
);

console.log({ actionListOptions });

React.useEffect(() => {
setOptions(actionListOptions);
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,4 @@ const _ActionListBox = React.forwardRef<SectionList, ActionListBoxProps>(

const ActionListBox = assignWithoutSideEffects(_ActionListBox, { displayName: 'ActionListBox' });

export { ActionListBox };
export { ActionListBox, ActionListBox as ActionListVirtualizedBox };
126 changes: 124 additions & 2 deletions packages/blade/src/components/ActionList/ActionListBox.web.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
/* eslint-disable react/display-name */
import React from 'react';
import { FixedSizeList as VirtualizedList } from 'react-window';
import { StyledListBoxWrapper } from './styles/StyledListBoxWrapper';
import type { SectionData } from './actionListUtils';
import { actionListMaxHeight, getActionListPadding } from './styles/getBaseListBoxWrapperStyles';
import { useBottomSheetContext } from '~components/BottomSheet/BottomSheetContext';
import { assignWithoutSideEffects } from '~utils/assignWithoutSideEffects';
import { makeAccessible } from '~utils/makeAccessible';
import type { DataAnalyticsAttribute } from '~utils/types';
import { makeAnalyticsAttribute } from '~utils/makeAnalyticsAttribute';
import { useIsMobile } from '~utils/useIsMobile';
import { getItemHeight } from '~components/BaseMenu/BaseMenuItem/tokens';
import { useTheme } from '~utils';
import type { Theme } from '~components/BladeProvider';
import { useDropdown } from '~components/Dropdown/useDropdown';
import { dropdownComponentIds } from '~components/Dropdown/dropdownComponentIds';

type ActionListBoxProps = {
childrenWithId?: React.ReactNode[] | null;
Expand Down Expand Up @@ -36,6 +44,120 @@ const _ActionListBox = React.forwardRef<HTMLDivElement, ActionListBoxProps>(
},
);

const ActionListBox = assignWithoutSideEffects(_ActionListBox, { displayName: 'ActionListBox' });
const ActionListBox = assignWithoutSideEffects(React.memo(_ActionListBox), {
displayName: 'ActionListBox',
});

export { ActionListBox };
const getVirtualItemParams = ({
theme,
isMobile,
}: {
theme: Theme;
isMobile: boolean;
}): {
itemHeight: number;
actionListBoxHeight: number;
} => {
const itemHeightResponsive = getItemHeight(theme);
const actionListPadding = getActionListPadding(theme);
const actionListBoxHeight = actionListMaxHeight - actionListPadding * 2;

return {
itemHeight: isMobile
? itemHeightResponsive.itemHeightMobile
: itemHeightResponsive.itemHeightDesktop,
actionListBoxHeight,
};
};

const useFilteredItems = (
saurabhdaware marked this conversation as resolved.
Show resolved Hide resolved
children: React.ReactNode[],
): {
itemData: React.ReactNode[];
itemCount: number;
} => {
const childrenArray = React.Children.toArray(children); // Convert children to an array

const { filteredValues, hasAutoCompleteInBottomSheetHeader, dropdownTriggerer } = useDropdown();

const items = React.useMemo(() => {
const hasAutoComplete =
hasAutoCompleteInBottomSheetHeader ||
dropdownTriggerer === dropdownComponentIds.triggers.AutoComplete;

if (!hasAutoComplete) {
return childrenArray;
}

// @ts-expect-error: props does exist
const filteredItems = childrenArray.filter((item) => filteredValues.includes(item.props.value));
return filteredItems;
}, [filteredValues, hasAutoCompleteInBottomSheetHeader, dropdownTriggerer, childrenArray]);

return {
itemData: items,
itemCount: items.length,
};
};

const VirtualListItem = ({
index,
style,
data,
}: {
index: number;
style: React.CSSProperties;
data: React.ReactNode[];
}): React.ReactElement => {
return <div style={style}>{data[index]}</div>;
};

const _ActionListVirtualizedBox = React.forwardRef<HTMLDivElement, ActionListBoxProps>(
({ childrenWithId, actionListItemWrapperRole, isMultiSelectable, ...rest }, ref) => {
const items = React.Children.toArray(childrenWithId); // Convert children to an array
const { isInBottomSheet } = useBottomSheetContext();
const { itemData, itemCount } = useFilteredItems(items);

const isMobile = useIsMobile();
const { theme } = useTheme();
const { itemHeight, actionListBoxHeight } = React.useMemo(
() => getVirtualItemParams({ theme, isMobile }),
// eslint-disable-next-line react-hooks/exhaustive-deps
[theme.name, isMobile],
);

return (
<StyledListBoxWrapper
isInBottomSheet={isInBottomSheet}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try it out with a bottomsheet once in a real device. Ensure there is no weird issues

ref={ref}
{...makeAccessible({
role: actionListItemWrapperRole,
multiSelectable: actionListItemWrapperRole === 'listbox' ? isMultiSelectable : undefined,
})}
{...makeAnalyticsAttribute(rest)}
>
{itemCount < 30 ? (
childrenWithId
) : (
<VirtualizedList
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 We ignore isVirtuzaliedList prop if itemCount is less than 30?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduced it to 10 items

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can log an info that if item count is less switching to non virtualized list , might lead to a different experience in dev or prod env. if itemCount is less.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh true good call. Will probably reduce the itemCount number and add a note in docs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note in ActionList docs props table. Will add better note when we write docs and tests and virtulization. Currently I have not added any documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might lead to a different experience in dev or prod env. if itemCount is less.

Let's wrap it in DEV flag, otherwise consumers may suddenly get lot of noise in their consoles.

height={actionListBoxHeight}
width="100%"
itemSize={itemHeight}
itemCount={itemCount}
itemData={itemData}
// @ts-expect-error: props does exist
itemKey={(index) => itemData[index]?.props.value}
>
{VirtualListItem}
</VirtualizedList>
)}
</StyledListBoxWrapper>
);
},
);

const ActionListVirtualizedBox = assignWithoutSideEffects(React.memo(_ActionListVirtualizedBox), {
displayName: 'ActionListVirtualizedBox',
});

export { ActionListBox, ActionListVirtualizedBox };
5 changes: 3 additions & 2 deletions packages/blade/src/components/ActionList/ActionListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -348,10 +348,11 @@ const _ActionListItem = (props: ActionListItemProps): React.ReactElement => {
}
}, [props.intent, dropdownTriggerer]);

const isVisible = hasAutoComplete && filteredValues ? filteredValues.includes(props.value) : true;

return (
// We use this context to change the color of subcomponents like ActionListItemIcon, ActionListItemText, etc
<BaseMenuItem
isVisible={hasAutoComplete && filteredValues ? filteredValues.includes(props.value) : true}
isVisible={isVisible}
as={!isReactNative() ? renderOnWebAs : undefined}
id={`${dropdownBaseId}-${props._index}`}
tabIndex={-1}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const actionListPropsTables: {
<ScrollLink href="#actionlistsection">&lt;ActionListSection[] /&gt;</ScrollLink>
</>
),
isVirtualized: 'boolean',
},
ActionListItem: {
title: 'string',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,20 @@ import type { Theme } from '~components/BladeProvider';
import { makeSize } from '~utils/makeSize';
import { size } from '~tokens/global';

const actionListMaxHeight = size[300];

const getActionListPadding = (theme: Theme): number => {
return theme.spacing[3];
};

const getBaseListBoxWrapperStyles = (props: {
theme: Theme;
isInBottomSheet: boolean;
}): CSSObject => {
return {
maxHeight: props.isInBottomSheet ? undefined : makeSize(size[300]),
padding: props.isInBottomSheet ? undefined : makeSize(props.theme.spacing[3]),
maxHeight: props.isInBottomSheet ? undefined : makeSize(actionListMaxHeight),
padding: props.isInBottomSheet ? undefined : makeSize(getActionListPadding(props.theme)),
};
};

export { getBaseListBoxWrapperStyles };
export { getBaseListBoxWrapperStyles, actionListMaxHeight, getActionListPadding };
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import React from 'react';
import type { BaseMenuItemProps } from '../types';
import { BaseMenuItemContext } from '../BaseMenuContext';
import { StyledMenuItemContainer } from './StyledMenuItemContainer';
import { itemFirstRowHeight } from './tokens';
import { Box } from '~components/Box';
import { getTextProps, Text } from '~components/Typography';
import { size } from '~tokens/global';
import { makeSize } from '~utils';
import { makeAccessible } from '~utils/makeAccessible';
import type { BladeElementRef } from '~utils/types';
import { BaseText } from '~components/Typography/BaseText';
import { useTruncationTitle } from '~utils/useTruncationTitle';
import { makeSize } from '~utils';

const menuItemTitleColor = {
negative: {
Expand All @@ -25,7 +25,6 @@ const menuItemDescriptionColor = {
} as const;

// This is the height of item excluding the description to make sure description comes at the bottom and other first row items are center aligned
const itemFirstRowHeight = makeSize(size[20]);

const _BaseMenuItem: React.ForwardRefRenderFunction<BladeElementRef, BaseMenuItemProps> = (
{
Expand Down Expand Up @@ -75,7 +74,7 @@ const _BaseMenuItem: React.ForwardRefRenderFunction<BladeElementRef, BaseMenuIte
display="flex"
justifyContent="center"
alignItems="center"
height={itemFirstRowHeight}
height={makeSize(itemFirstRowHeight)}
>
{leading}
</Box>
Expand All @@ -89,7 +88,7 @@ const _BaseMenuItem: React.ForwardRefRenderFunction<BladeElementRef, BaseMenuIte
display="flex"
alignItems="center"
flexDirection="row"
height={itemFirstRowHeight}
height={makeSize(itemFirstRowHeight)}
ref={containerRef as never}
>
<BaseText
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
import styled from 'styled-components';
import type { StyledBaseMenuItemContainerProps } from '../types';
import { getBaseMenuItemStyles } from './getBaseMenuItemStyles';
import { getItemPadding } from './tokens';
import { getMediaQuery, makeSize } from '~utils';
import { getFocusRingStyles } from '~utils/getFocusRingStyles';
import BaseBox from '~components/Box/BaseBox';

const StyledMenuItemContainer = styled(BaseBox)<StyledBaseMenuItemContainerProps>((props) => {
return {
...getBaseMenuItemStyles({ theme: props.theme }),
padding: makeSize(props.theme.spacing[2]),
padding: makeSize(getItemPadding(props.theme).itemPaddingMobile),
display: props.isVisible ? 'flex' : 'none',
[`@media ${getMediaQuery({ min: props.theme.breakpoints.m })}`]: {
padding: makeSize(props.theme.spacing[3]),
padding: makeSize(getItemPadding(props.theme).itemPaddingDesktop),
},
'&:hover:not([aria-disabled=true]), &[aria-expanded="true"]': {
backgroundColor:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { CSSObject } from 'styled-components';
import { getItemMargin } from './tokens';
import type { Theme } from '~components/BladeProvider';
import { isReactNative, makeBorderSize } from '~utils';
import { makeSize } from '~utils/makeSize';
Expand All @@ -11,8 +12,8 @@ const getBaseMenuItemStyles = (props: { theme: Theme }): CSSObject => {
textAlign: isReactNative() ? undefined : 'left',
backgroundColor: 'transparent',
borderRadius: makeSize(props.theme.border.radius.medium),
marginTop: makeSize(props.theme.spacing[1]),
marginBottom: makeSize(props.theme.spacing[1]),
marginTop: makeSize(getItemMargin(props.theme)),
marginBottom: makeSize(getItemMargin(props.theme)),
textDecoration: 'none',
cursor: 'pointer',
width: '100%',
Expand Down
38 changes: 38 additions & 0 deletions packages/blade/src/components/BaseMenu/BaseMenuItem/tokens.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import type { Theme } from '~components/BladeProvider';
import { size } from '~tokens/global';

const itemFirstRowHeight = size[20];

const getItemPadding = (
theme: Theme,
): {
itemPaddingMobile: 4;
itemPaddingDesktop: 8;
} => {
return {
itemPaddingMobile: theme.spacing[2],
itemPaddingDesktop: theme.spacing[3],
};
};

const getItemMargin = (theme: Theme): number => {
return theme.spacing[1];
};

const getItemHeight = (
theme: Theme,
): {
itemHeightMobile: number;
itemHeightDesktop: number;
} => {
return {
itemHeightMobile:
// eslint-disable-next-line @typescript-eslint/restrict-plus-operands
itemFirstRowHeight + getItemPadding(theme).itemPaddingMobile * 2 + getItemMargin(theme) * 2,
itemHeightDesktop:
// eslint-disable-next-line @typescript-eslint/restrict-plus-operands
anuraghazra marked this conversation as resolved.
Show resolved Hide resolved
itemFirstRowHeight + getItemPadding(theme).itemPaddingDesktop * 2 + getItemMargin(theme) * 2,
};
};

export { itemFirstRowHeight, getItemPadding, getItemMargin, getItemHeight };
Loading
Loading