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

Selection List refactor phase 3: base #27767

Merged
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
beca6d0
chore: wip
thiagobrez Sep 13, 2023
478d731
Merge branch 'main' into thiagobrez/selection-list-phase-3-base
thiagobrez Sep 13, 2023
37f66e4
refactor(selection-list): create BaseListItem
thiagobrez Sep 13, 2023
0b12a94
chore: resolve conflicts
thiagobrez Sep 15, 2023
9d3cb2e
feat(selection-list): request money
thiagobrez Sep 18, 2023
5978ba7
chore: resolve conflicts
thiagobrez Sep 18, 2023
a74b3bd
chore: rollback request money and split bill changes
thiagobrez Sep 19, 2023
fc74d8d
chore: rollback request money and split bill changes
thiagobrez Sep 19, 2023
b652bb6
chore: rollback request money and split bill changes
thiagobrez Sep 19, 2023
0bc58a4
chore: fix PropTYpes
thiagobrez Sep 19, 2023
0b2a660
chore: fix import
thiagobrez Sep 19, 2023
0b31679
chore: rollback deleted function
thiagobrez Sep 19, 2023
e1ecc08
chore: address pr comments
thiagobrez Sep 19, 2023
68be6e9
chore: fix conflicts
thiagobrez Sep 21, 2023
b629c22
chore: resolve conflicts
thiagobrez Sep 27, 2023
ae0ce3d
chore: rename variable
thiagobrez Sep 27, 2023
02a8af9
chore: remove useArrowKeyFocusManager hook in favor of component
thiagobrez Sep 28, 2023
300a092
chore: fix comment
thiagobrez Sep 28, 2023
c126fc3
refactor(tooltip): add ability to prevent render
thiagobrez Sep 28, 2023
424457d
chore: fix tooltip props
thiagobrez Sep 28, 2023
4f74105
test: fix formatMemberForList tests
thiagobrez Sep 28, 2023
b387ed8
chore: add proptypes comments
thiagobrez Sep 29, 2023
158e8a1
chore: add proptypes comments
thiagobrez Sep 29, 2023
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
100 changes: 100 additions & 0 deletions src/components/SelectionList/BaseListItem.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import React from 'react';
import {View} from 'react-native';
import lodashGet from 'lodash/get';
import PressableWithFeedback from '../Pressable/PressableWithFeedback';
import styles from '../../styles/styles';
import Icon from '../Icon';
import * as Expensicons from '../Icon/Expensicons';
import themeColors from '../../styles/themes/default';
import {baseListItemPropTypes} from './selectionListPropTypes';
import * as StyleUtils from '../../styles/StyleUtils';
import UserListItem from './UserListItem';
import RadioListItem from './RadioListItem';
import OfflineWithFeedback from '../OfflineWithFeedback';
import CONST from '../../CONST';

function BaseListItem({item, isFocused = false, isDisabled = false, showTooltip, canSelectMultiple = false, onSelectRow, onDismissError = () => {}}) {
const isUserItem = lodashGet(item, 'icons.length', 0) > 0;
const ListItem = isUserItem ? UserListItem : RadioListItem;

return (
<OfflineWithFeedback
onClose={() => onDismissError(item)}
pendingAction={item.pendingAction}
errors={item.errors}
errorRowStyles={styles.ph5}
>
<PressableWithFeedback
onPress={() => onSelectRow(item)}
disabled={isDisabled}
accessibilityLabel={item.text}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
hoverDimmingValue={1}
hoverStyle={styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
>
<View
style={[
styles.flex1,
styles.justifyContentBetween,
styles.sidebarLinkInner,
styles.userSelectNone,
isUserItem ? styles.peopleRow : styles.optionRow,
isFocused && styles.sidebarLinkActive,
]}
>
{canSelectMultiple && (
<View style={styles.checkboxPressable}>
<View
style={[
StyleUtils.getCheckboxContainerStyle(20, 4),
styles.mr3,
item.isSelected && styles.checkedContainer,
item.isSelected && styles.borderColorFocus,
item.isDisabled && styles.cursorDisabled,
item.isDisabled && styles.buttonOpacityDisabled,
]}
>
{item.isSelected && (
<Icon
src={Expensicons.Checkmark}
fill={themeColors.textLight}
height={14}
width={14}
/>
)}
</View>
</View>
)}

<ListItem
item={item}
isFocused={isFocused}
isDisabled={isDisabled}
onSelectRow={onSelectRow}
showTooltip={showTooltip}
/>

{!canSelectMultiple && item.isSelected && (
<View
style={[styles.flexRow, styles.alignItemsCenter, styles.ml3]}
accessible={false}
>
<View>
<Icon
src={Expensicons.Checkmark}
fill={themeColors.success}
/>
</View>
</View>
)}
</View>
</PressableWithFeedback>
</OfflineWithFeedback>
);
}

BaseListItem.displayName = 'BaseListItem';
BaseListItem.propTypes = baseListItemPropTypes;

export default BaseListItem;
91 changes: 53 additions & 38 deletions src/components/SelectionList/BaseSelectionList.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,9 @@ import SectionList from '../SectionList';
import Text from '../Text';
import styles from '../../styles/styles';
import TextInput from '../TextInput';
import ArrowKeyFocusManager from '../ArrowKeyFocusManager';
import CONST from '../../CONST';
import variables from '../../styles/variables';
import {propTypes as selectionListPropTypes} from './selectionListPropTypes';
import RadioListItem from './RadioListItem';
import UserListItem from './UserListItem';
import useKeyboardShortcut from '../../hooks/useKeyboardShortcut';
import SafeAreaConsumer from '../SafeAreaConsumer';
import withKeyboardState, {keyboardStatePropTypes} from '../withKeyboardState';
Expand All @@ -24,6 +21,9 @@ import useLocalize from '../../hooks/useLocalize';
import Log from '../../libs/Log';
import OptionsListSkeletonView from '../OptionsListSkeletonView';
import useActiveElement from '../../hooks/useActiveElement';
import BaseListItem from './BaseListItem';
import themeColors from '../../styles/themes/default';
import ArrowKeyFocusManager from '../ArrowKeyFocusManager';

const propTypes = {
...keyboardStatePropTypes,
Expand All @@ -48,10 +48,13 @@ function BaseSelectionList({
headerMessage = '',
confirmButtonText = '',
onConfirm,
footerContent,
showScrollIndicator = false,
showLoadingPlaceholder = false,
showConfirmButton = false,
isKeyboardShown = false,
disableKeyboardShortcuts = false,
children,
}) {
const {translate} = useLocalize();
const firstLayoutRef = useRef(true);
Expand Down Expand Up @@ -136,19 +139,19 @@ function BaseSelectionList({
};
}, [canSelectMultiple, sections]);

// Disable `Enter` hotkey if the active element is a button or checkbox
const shouldDisableHotkeys = activeElement && [CONST.ACCESSIBILITY_ROLE.BUTTON, CONST.ACCESSIBILITY_ROLE.CHECKBOX].includes(activeElement.role);

// If `initiallyFocusedOptionKey` is not passed, we fall back to `-1`, to avoid showing the highlight on the first member
const [focusedIndex, setFocusedIndex] = useState(() => _.findIndex(flattenedSections.allOptions, (option) => option.keyForList === initiallyFocusedOptionKey));

// Disable `Enter` shortcut if the active element is a button or checkbox
const disableEnterShortcut = activeElement && [CONST.ACCESSIBILITY_ROLE.BUTTON, CONST.ACCESSIBILITY_ROLE.CHECKBOX].includes(activeElement.role);

/**
* Scrolls to the desired item index in the section list
*
* @param {Number} index - the index of the item to scroll to
* @param {Boolean} animated - whether to animate the scroll
*/
const scrollToIndex = (index, animated) => {
const scrollToIndex = useCallback((index, animated = true) => {
const item = flattenedSections.allOptions[index];

if (!listRef.current || !item) {
Expand All @@ -169,7 +172,10 @@ function BaseSelectionList({
}

listRef.current.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated, viewOffset: variables.contentHeaderHeight});
};

// If we don't disable dependencies here, we would need to make sure that the `sections` prop is stable in every usage of this component.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Copy link
Member

Choose a reason for hiding this comment

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

This change caused a bug - WS - Members - List of users invited to the workspace does not scroll with Up and Down arrows

allOptions is initially empty. But this callback runs only on mount, so it doesn't change when allOptions are populated.


/**
* Logic to run when a row is selected, either with click/press or keyboard hotkeys.
Expand Down Expand Up @@ -234,6 +240,14 @@ function BaseSelectionList({
const getItemLayout = (data, flatDataArrayIndex) => {
const targetItem = flattenedSections.itemLayouts[flatDataArrayIndex];

if (!targetItem) {
return {
length: 0,
offset: 0,
index: flatDataArrayIndex,
};
}

return {
length: targetItem.length,
offset: targetItem.offset,
Expand All @@ -259,33 +273,40 @@ function BaseSelectionList({

const renderItem = ({item, index, section}) => {
const normalizedIndex = index + lodashGet(section, 'indexOffset', 0);
const isDisabled = section.isDisabled;
const isDisabled = section.isDisabled || item.isDisabled;
const isItemFocused = !isDisabled && focusedIndex === normalizedIndex;
// We only create tooltips for the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
const showTooltip = normalizedIndex < 10;

if (canSelectMultiple) {
return (
<UserListItem
item={item}
isFocused={isItemFocused}
onSelectRow={() => selectRow(item, true)}
onDismissError={onDismissError}
showTooltip={showTooltip}
/>
);
}

return (
<RadioListItem
<BaseListItem
item={item}
isFocused={isItemFocused}
isDisabled={isDisabled}
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={() => selectRow(item, true)}
onDismissError={onDismissError}
/>
);
};

const scrollToFocusedIndexOnFirstRender = useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like introducing this caused issue: #31660 (fixed by this PR: #31737)

if (!firstLayoutRef.current) {
return;
}
scrollToIndex(focusedIndex, false);
firstLayoutRef.current = false;
}, [focusedIndex, scrollToIndex]);

const updateAndScrollToFocusedIndex = useCallback(
(newFocusedIndex) => {
setFocusedIndex(newFocusedIndex);
scrollToIndex(newFocusedIndex, true);
},
[scrollToIndex],
);

/** Focuses the text input when the component comes into focus and after any navigation animations finish. */
useFocusEffect(
useCallback(() => {
Expand All @@ -305,25 +326,22 @@ function BaseSelectionList({
useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
captureOnInputs: true,
shouldBubble: () => !flattenedSections.allOptions[focusedIndex],
isActive: !shouldDisableHotkeys && isFocused,
isActive: !disableKeyboardShortcuts && !disableEnterShortcut && isFocused,
});

/** Calls confirm action when pressing CTRL (CMD) + Enter */
useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onConfirm, {
captureOnInputs: true,
shouldBubble: () => !flattenedSections.allOptions[focusedIndex],
isActive: Boolean(onConfirm) && isFocused,
isActive: !disableKeyboardShortcuts && Boolean(onConfirm) && isFocused,
});

return (
<ArrowKeyFocusManager
thiagobrez marked this conversation as resolved.
Show resolved Hide resolved
disabledIndexes={flattenedSections.disabledOptionsIndexes}
focusedIndex={focusedIndex}
maxIndex={flattenedSections.allOptions.length - 1}
onFocusedIndexChanged={(newFocusedIndex) => {
setFocusedIndex(newFocusedIndex);
scrollToIndex(newFocusedIndex, true);
}}
onFocusedIndexChanged={updateAndScrollToFocusedIndex}
>
<SafeAreaConsumer>
{({safeAreaPaddingBottomStyle}) => (
Expand Down Expand Up @@ -360,7 +378,7 @@ function BaseSelectionList({
style={[styles.peopleRow, styles.userSelectNone, styles.ph5, styles.pb3]}
onPress={onSelectAll}
accessibilityLabel={translate('workspace.people.selectAll')}
accessibilityRole="button"
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
accessibilityState={{checked: flattenedSections.allSelected}}
disabled={flattenedSections.allOptions.length === flattenedSections.disabledOptionsIndexes.length}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
Expand All @@ -387,26 +405,22 @@ function BaseSelectionList({
onScrollBeginDrag={onScrollBeginDrag}
keyExtractor={(item) => item.keyForList}
extraData={focusedIndex}
indicatorStyle="white"
indicatorStyle={themeColors.selectionListIndicatorColor}
keyboardShouldPersistTaps="always"
showsVerticalScrollIndicator={showScrollIndicator}
initialNumToRender={12}
maxToRenderPerBatch={5}
windowSize={5}
viewabilityConfig={{viewAreaCoveragePercentThreshold: 95}}
testID="selection-list"
onLayout={() => {
if (!firstLayoutRef.current) {
return;
}
scrollToIndex(focusedIndex, false);
firstLayoutRef.current = false;
}}
style={[styles.flexGrow0]}
Copy link
Contributor

Choose a reason for hiding this comment

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

This caused minor scrolling issue related to iOS bounce - #30428

onLayout={scrollToFocusedIndexOnFirstRender}
/>
{children}
</>
)}
{showConfirmButton && (
<FixedFooter>
<FixedFooter style={[styles.mtAuto]}>
<Button
success
style={[styles.w100]}
Expand All @@ -417,6 +431,7 @@ function BaseSelectionList({
/>
</FixedFooter>
)}
{Boolean(footerContent) && <FixedFooter style={[styles.mtAuto]}>{footerContent}</FixedFooter>}
</View>
)}
</SafeAreaConsumer>
Expand Down
47 changes: 7 additions & 40 deletions src/components/SelectionList/RadioListItem.js
Original file line number Diff line number Diff line change
@@ -1,51 +1,18 @@
import React from 'react';
import {View} from 'react-native';
import CONST from '../../CONST';
import PressableWithFeedback from '../Pressable/PressableWithFeedback';
import styles from '../../styles/styles';
import Text from '../Text';
import Icon from '../Icon';
import * as Expensicons from '../Icon/Expensicons';
import themeColors from '../../styles/themes/default';
import {radioListItemPropTypes} from './selectionListPropTypes';

function RadioListItem({item, isFocused = false, isDisabled = false, onSelectRow}) {
function RadioListItem({item, isFocused = false}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the Checkmark part and moved to BaseListItem

return (
<PressableWithFeedback
onPress={() => onSelectRow(item)}
disabled={isDisabled}
accessibilityLabel={item.text}
accessibilityRole="button"
hoverDimmingValue={1}
hoverStyle={styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
>
<View style={[styles.flex1, styles.justifyContentBetween, styles.sidebarLinkInner, styles.optionRow, styles.userSelectNone, isFocused && styles.sidebarLinkActive]}>
<View style={[styles.flex1, styles.alignItemsStart]}>
<Text style={[styles.optionDisplayName, isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText, item.isSelected && styles.sidebarLinkTextBold]}>
{item.text}
</Text>
<View style={[styles.flex1, styles.alignItemsStart]}>
<Text style={[styles.optionDisplayName, isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText, item.isSelected && styles.sidebarLinkTextBold]}>{item.text}</Text>
Copy link
Contributor

@s77rt s77rt Dec 13, 2023

Choose a reason for hiding this comment

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

We had to specify the numberOfLines={1} to show ellipsis if the text is too long instead of having it overflow (Coming from #32384)


{Boolean(item.alternateText) && (
<Text style={[isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText, styles.optionAlternateText, styles.textLabelSupporting]}>{item.alternateText}</Text>
)}
</View>

{item.isSelected && (
<View
style={[styles.flexRow, styles.alignItemsCenter]}
accessible={false}
>
<View>
<Icon
src={Expensicons.Checkmark}
fill={themeColors.success}
/>
</View>
</View>
)}
</View>
</PressableWithFeedback>
{Boolean(item.alternateText) && (
<Text style={[isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText, styles.optionAlternateText, styles.textLabelSupporting]}>{item.alternateText}</Text>
)}
</View>
);
}

Expand Down
Loading
Loading