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.

Sign up for GitHub

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

Jump to bottom

#35712 selection list refactor #37000

Merged
merged 21 commits into from
Feb 21, 2024
Merged

#35712 selection list refactor #37000

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0348111
render item prop exposed
burczu Feb 20, 2024
9b44d95
render item prop handled where selection list used
burczu Feb 20, 2024
bc397c3
list item definition extracted from base list item
burczu Feb 20, 2024
65abacf
list item styling moved to specific list style elements instead of ba…
burczu Feb 20, 2024
1551fac
base list items used by specific list elements instead coupling with …
burczu Feb 20, 2024
eba4f51
prettier and lint fixes
burczu Feb 20, 2024
5fdb270
type mismatch fixed
burczu Feb 20, 2024
277e565
ability to use children explicitly added
burczu Feb 20, 2024
ed67986
wrapper and select multiple styles exposed outside the base list item
burczu Feb 20, 2024
d64e8a1
prettier fixes applied
burczu Feb 20, 2024
c4a621b
switched from renderItem to ListItem prop
burczu Feb 20, 2024
7697efc
used correct ListItem in all the confusing places
burczu Feb 21, 2024
9fbf025
Merge branch 'main' into refactor/35712-selection-list-refactor
burczu Feb 21, 2024
909ae4d
selection list performance test fixed
burczu Feb 21, 2024
66ba2dd
prettier fixed
burczu Feb 21, 2024
432a450
unnecessary commented code removed
burczu Feb 21, 2024
742d5f2
allowing only expected types as ListItem
burczu Feb 21, 2024
24ff7de
prettier, one more time
burczu Feb 21, 2024
caeed9a
user specific props added
burczu Feb 21, 2024
01d08cd
common styles extracted to StyleUtils
burczu Feb 21, 2024
45fd85b
missing comment added
burczu Feb 21, 2024
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
2 changes: 2 additions & 0 deletions src/components/DatePicker/CalendarPicker/YearPickerModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import HeaderWithBackButton from '@components/HeaderWithBackButton';
import Modal from '@components/Modal';
import ScreenWrapper from '@components/ScreenWrapper';
import SelectionList from '@components/SelectionList';
import RadioListItem from '@components/SelectionList/RadioListItem';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import CONST from '@src/CONST';
Expand Down Expand Up @@ -79,6 +80,7 @@ function YearPickerModal({isVisible, years, currentYear = new Date().getFullYear
showScrollIndicator
shouldStopPropagation
shouldUseDynamicMaxToRenderPerBatch
ListItem={RadioListItem}
/>
</ScreenWrapper>
</Modal>
Expand Down
47 changes: 6 additions & 41 deletions src/components/SelectionList/BaseListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,26 @@ import useStyleUtils from '@hooks/useStyleUtils';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import CONST from '@src/CONST';
import RadioListItem from './RadioListItem';
import type {BaseListItemProps, ListItem} from './types';
import UserListItem from './UserListItem';

function BaseListItem<TItem extends ListItem>({
item,
isFocused = false,
wrapperStyle,
selectMultipleStyle,
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
isDisabled = false,
showTooltip,
shouldPreventDefaultFocusOnSelectRow = false,
canSelectMultiple = false,
onSelectRow,
onDismissError = () => {},
rightHandSideComponent,
keyForList,
children,
}: BaseListItemProps<TItem>) {
const theme = useTheme();
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const {translate} = useLocalize();
const isUserItem = 'icons' in item && item?.icons?.length && item.icons.length > 0;
burczu marked this conversation as resolved.
Show resolved Hide resolved
const ListItem = isUserItem ? UserListItem : RadioListItem;

const rightHandSideComponentRender = () => {
if (canSelectMultiple || !rightHandSideComponent) {
Expand Down Expand Up @@ -65,31 +63,13 @@ function BaseListItem<TItem extends ListItem>({
>
{({hovered}) => (
<>
<View
style={[
styles.flex1,
styles.justifyContentBetween,
styles.sidebarLinkInner,
styles.userSelectNone,
isUserItem ? styles.peopleRow : styles.optionRow,
isFocused && styles.sidebarLinkActive,
]}
>
<View style={wrapperStyle}>
{canSelectMultiple && (
<View
role={CONST.ACCESSIBILITY_ROLE.BUTTON}
style={StyleUtils.getCheckboxPressableStyle()}
>
<View
style={[
StyleUtils.getCheckboxContainerStyle(20),
styles.mr3,
item.isSelected && styles.checkedContainer,
item.isSelected && styles.borderColorFocus,
item.isDisabled && styles.cursorDisabled,
item.isDisabled && styles.buttonOpacityDisabled,
]}
>
<View style={selectMultipleStyle}>
{item.isSelected && (
<Icon
src={Expensicons.Checkmark}
Expand All @@ -102,22 +82,7 @@ function BaseListItem<TItem extends ListItem>({
</View>
)}

<ListItem
item={item}
textStyles={[
styles.optionDisplayName,
isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
styles.sidebarLinkTextBold,
styles.pre,
item.alternateText ? styles.mb1 : null,
]}
alternateTextStyles={[styles.textLabelSupporting, styles.lh16, styles.pre]}
isDisabled={isDisabled}
onSelectRow={() => onSelectRow(item)}
showTooltip={showTooltip}
isFocused={isFocused}
isHovered={hovered}
/>
{typeof children === 'function' ? children(hovered) : children}

{!canSelectMultiple && item.isSelected && !rightHandSideComponent && (
<View
Expand Down
4 changes: 2 additions & 2 deletions src/components/SelectionList/BaseSelectionList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ import Log from '@libs/Log';
import variables from '@styles/variables';
import CONST from '@src/CONST';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import BaseListItem from './BaseListItem';
import type {BaseSelectionListProps, ButtonOrCheckBoxRoles, FlattenedSectionsReturn, ListItem, Section, SectionListDataType} from './types';

function BaseSelectionList<TItem extends ListItem>(
{
sections,
ListItem,
burczu marked this conversation as resolved.
Show resolved Hide resolved
canSelectMultiple = false,
onSelectRow,
onSelectAll,
Expand Down Expand Up @@ -280,7 +280,7 @@ function BaseSelectionList<TItem extends ListItem>(
const showTooltip = shouldShowTooltips && normalizedIndex < 10;

return (
<BaseListItem
<ListItem
item={item}
isFocused={isItemFocused}
isDisabled={isDisabled}
Expand Down
69 changes: 55 additions & 14 deletions src/components/SelectionList/RadioListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,69 @@
import React from 'react';
import {View} from 'react-native';
import TextWithTooltip from '@components/TextWithTooltip';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
import type {ListItemProps} from './types';
import BaseListItem from './BaseListItem';
import type {BaseListItemProps, ListItem} from './types';

function RadioListItem({item, showTooltip, textStyles, alternateTextStyles}: ListItemProps) {
function RadioListItem({
item,
isFocused,
showTooltip,
isDisabled,
canSelectMultiple,
onSelectRow,
onDismissError,
shouldPreventDefaultFocusOnSelectRow,
rightHandSideComponent,
}: BaseListItemProps<ListItem>) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();

return (
<View style={[styles.flex1, styles.alignItemsStart]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.text}
textStyles={textStyles}
/>

{!!item.alternateText && (
<BaseListItem
item={item}
wrapperStyle={[styles.flex1, styles.justifyContentBetween, styles.sidebarLinkInner, styles.userSelectNone, styles.optionRow, isFocused && styles.sidebarLinkActive]}
selectMultipleStyle={[
StyleUtils.getCheckboxContainerStyle(20),
styles.mr3,
item.isSelected && styles.checkedContainer,
item.isSelected && styles.borderColorFocus,
item.isDisabled && styles.cursorDisabled,
item.isDisabled && styles.buttonOpacityDisabled,
burczu marked this conversation as resolved.
Show resolved Hide resolved
]}
isFocused={isFocused}
isDisabled={isDisabled}
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={onSelectRow}
onDismissError={onDismissError}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
rightHandSideComponent={rightHandSideComponent}
keyForList={item.keyForList}
>
<View style={[styles.flex1, styles.alignItemsStart]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.alternateText}
textStyles={alternateTextStyles}
text={item.text}
textStyles={[
styles.optionDisplayName,
isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
styles.sidebarLinkTextBold,
styles.pre,
item.alternateText ? styles.mb1 : null,
]}
/>
)}
</View>

{!!item.alternateText && (
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.alternateText}
textStyles={[styles.textLabelSupporting, styles.lh16, styles.pre]}
/>
)}
</View>
</BaseListItem>
);
}

Expand Down
111 changes: 76 additions & 35 deletions src/components/SelectionList/UserListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,20 @@ import TextWithTooltip from '@components/TextWithTooltip';
import useStyleUtils from '@hooks/useStyleUtils';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import type {ListItemProps} from './types';
import BaseListItem from './BaseListItem';
import type {BaseListItemProps, ListItem} from './types';

function UserListItem({item, textStyles, alternateTextStyles, showTooltip, style, isFocused, isHovered}: ListItemProps) {
function UserListItem({
item,
isFocused,
showTooltip,
isDisabled,
canSelectMultiple,
onSelectRow,
onDismissError,
shouldPreventDefaultFocusOnSelectRow,
rightHandSideComponent,
}: BaseListItemProps<ListItem>) {
const styles = useThemeStyles();
const theme = useTheme();
const StyleUtils = useStyleUtils();
Expand All @@ -18,45 +29,75 @@ function UserListItem({item, textStyles, alternateTextStyles, showTooltip, style
const hoveredBackgroundColor = !!styles.sidebarLinkHover && 'backgroundColor' in styles.sidebarLinkHover ? styles.sidebarLinkHover.backgroundColor : theme.sidebar;

return (
<>
{!!item.icons && (
<BaseListItem
item={item}
wrapperStyle={[styles.flex1, styles.justifyContentBetween, styles.sidebarLinkInner, styles.userSelectNone, styles.peopleRow, isFocused && styles.sidebarLinkActive]}
selectMultipleStyle={[
StyleUtils.getCheckboxContainerStyle(20),
styles.mr3,
item.isSelected && styles.checkedContainer,
item.isSelected && styles.borderColorFocus,
item.isDisabled && styles.cursorDisabled,
item.isDisabled && styles.buttonOpacityDisabled,
burczu marked this conversation as resolved.
Show resolved Hide resolved
]}
isFocused={isFocused}
isDisabled={isDisabled}
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={onSelectRow}
onDismissError={onDismissError}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
rightHandSideComponent={rightHandSideComponent}
keyForList={item.keyForList}
>
{(hovered) => (
<>
{item.shouldShowSubscript ? (
<SubscriptAvatar
mainAvatar={item.icons[0]}
secondaryAvatar={item.icons[1]}
showTooltip={showTooltip}
backgroundColor={isHovered && !isFocused ? hoveredBackgroundColor : subscriptAvatarBorderColor}
/>
) : (
<MultipleAvatars
icons={item.icons ?? []}
{!!item.icons && (
<>
{item.shouldShowSubscript ? (
<SubscriptAvatar
mainAvatar={item.icons[0]}
secondaryAvatar={item.icons[1]}
showTooltip={showTooltip}
backgroundColor={hovered && !isFocused ? hoveredBackgroundColor : subscriptAvatarBorderColor}
/>
) : (
<MultipleAvatars
icons={item.icons ?? []}
shouldShowTooltip={showTooltip}
secondAvatarStyle={[
StyleUtils.getBackgroundAndBorderStyle(theme.sidebar),
isFocused ? StyleUtils.getBackgroundAndBorderStyle(focusedBackgroundColor) : undefined,
hovered && !isFocused ? StyleUtils.getBackgroundAndBorderStyle(hoveredBackgroundColor) : undefined,
burczu marked this conversation as resolved.
Show resolved Hide resolved
]}
/>
)}
</>
)}
<View style={[styles.flex1, styles.flexColumn, styles.justifyContentCenter, styles.alignItemsStretch, styles.optionRow]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
secondAvatarStyle={[
StyleUtils.getBackgroundAndBorderStyle(theme.sidebar),
isFocused ? StyleUtils.getBackgroundAndBorderStyle(focusedBackgroundColor) : undefined,
isHovered && !isFocused ? StyleUtils.getBackgroundAndBorderStyle(hoveredBackgroundColor) : undefined,
text={item.text}
textStyles={[
styles.optionDisplayName,
isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
styles.sidebarLinkTextBold,
styles.pre,
item.alternateText ? styles.mb1 : null,
]}
/>
)}
{!!item.alternateText && (
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.alternateText}
textStyles={[styles.textLabelSupporting, styles.lh16, styles.pre]}
/>
)}
</View>
{!!item.rightElement && item.rightElement}
</>
)}
<View style={[styles.flex1, styles.flexColumn, styles.justifyContentCenter, styles.alignItemsStretch, styles.optionRow]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.text}
textStyles={[textStyles, style]}
Copy link
Contributor

Choose a reason for hiding this comment

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

style had strikethrough style from OfflineWithFeedback.
Removing this caused regression of strikethrough not applied on native when delete item offline

/>
{!!item.alternateText && (
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.alternateText}
textStyles={[alternateTextStyles, style]}
/>
)}
</View>
{!!item.rightElement && item.rightElement}
</>
</BaseListItem>
);
}

Expand Down
16 changes: 8 additions & 8 deletions src/components/SelectionList/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type {ReactElement, ReactNode} from 'react';
import type {JSXElementConstructor, ReactElement, ReactNode} from 'react';
import type {GestureResponderEvent, InputModeOptions, LayoutChangeEvent, SectionListData, StyleProp, TextStyle, ViewStyle} from 'react-native';
import type {Errors, Icon, PendingAction} from '@src/types/onyx/OnyxCommon';
import type ChildrenProps from '@src/types/utils/ChildrenProps';
Expand All @@ -7,12 +7,6 @@ type CommonListItemProps<TItem> = {
/** Whether this item is focused (for arrow key controls) */
isFocused?: boolean;

/** Style to be applied to Text */
textStyles?: StyleProp<TextStyle>;

/** Style to be applied on the alternate text */
alternateTextStyles?: StyleProp<TextStyle>;

/** Whether this item is disabled */
isDisabled?: boolean;

Expand Down Expand Up @@ -93,6 +87,9 @@ type BaseListItemProps<TItem extends ListItem> = CommonListItemProps<TItem> & {
item: TItem;
shouldPreventDefaultFocusOnSelectRow?: boolean;
keyForList?: string;
wrapperStyle?: StyleProp<ViewStyle>;
selectMultipleStyle?: StyleProp<ViewStyle>;
children?: ReactElement<ListItemProps> | ((hovered: boolean) => ReactElement<ListItemProps>);
};

type Section<TItem extends ListItem> = {
Expand All @@ -116,6 +113,9 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
/** Sections for the section list */
sections: Array<SectionListData<TItem, Section<TItem>>>;

/** Default renderer for every item in the list */
ListItem: JSXElementConstructor<BaseListItemProps<ListItem>>;

/** Whether this is a multi-select list */
canSelectMultiple?: boolean;

Expand Down Expand Up @@ -210,7 +210,7 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
shouldDelayFocus?: boolean;

/** Component to display on the right side of each child */
rightHandSideComponent?: ((item: TItem) => ReactElement<TItem>) | ReactElement | null;
rightHandSideComponent?: ((item: ListItem) => ReactElement<ListItem>) | ReactElement | null;

/** Whether to show the loading indicator for new options */
isLoadingNewOptions?: boolean;
Expand Down
Loading
Loading