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

Refactor/align selectionlist checkboxes #37402

Merged
merged 5 commits into from
Feb 28, 2024
Merged

Refactor/align selectionlist checkboxes #37402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
66a4c0a
feat: add optional checkbox press handled
ArekChr Feb 28, 2024
ad6aa38
chore: temporary navigate to categories settings when pressing row
ArekChr Feb 28, 2024
ee31536
fix: pass undefined if onCheckboxPress is undefined
ArekChr Feb 28, 2024
645a1a4
feat: add support for radio and user list item
ArekChr Feb 28, 2024
2e1472b
refactor: select all checkbox only in checkbox area
ArekChr Feb 28, 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
15 changes: 13 additions & 2 deletions src/components/SelectionList/BaseListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function BaseListItem<TItem extends ListItem>({
shouldPreventDefaultFocusOnSelectRow = false,
canSelectMultiple = false,
onSelectRow,
onCheckboxPress,
onDismissError = () => {},
rightHandSideComponent,
keyForList,
Expand All @@ -43,6 +44,14 @@ function BaseListItem<TItem extends ListItem>({
return rightHandSideComponent;
};

const handleCheckboxPress = () => {
Copy link
Contributor

@situchan situchan Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

NAB:

use useCallback with item dependency.

or I think onPress={() => (onCheckboxPress ?? onSelectRow)(item)} is better without defining this function

if (onCheckboxPress) {
onCheckboxPress(item);
} else {
onSelectRow(item);
}
};

return (
<OfflineWithFeedback
onClose={() => onDismissError(item)}
Expand All @@ -66,8 +75,10 @@ function BaseListItem<TItem extends ListItem>({
<>
<View style={wrapperStyle}>
{canSelectMultiple && (
<View
<PressableWithFeedback
accessibilityLabel={item.text}
Copy link
Contributor

Choose a reason for hiding this comment

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

a minor regression:

regression.mp4

we also need to pass disabled={isDisabled}? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I noticed that as well. Btw not blocker since admin cannot be actually removed.
Hope @ArekChr would fix it as follow-up along with my NAB feedback

role={CONST.ROLE.BUTTON}
onPress={handleCheckboxPress}
style={StyleUtils.getCheckboxPressableStyle()}
>
<View style={selectMultipleStyle}>
Expand All @@ -80,7 +91,7 @@ function BaseListItem<TItem extends ListItem>({
/>
)}
</View>
</View>
</PressableWithFeedback>
)}

{typeof children === 'function' ? children(hovered) : children}
Expand Down
18 changes: 5 additions & 13 deletions src/components/SelectionList/BaseSelectionList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import Button from '@components/Button';
import Checkbox from '@components/Checkbox';
import FixedFooter from '@components/FixedFooter';
import OptionsListSkeletonView from '@components/OptionsListSkeletonView';
import PressableWithFeedback from '@components/Pressable/PressableWithFeedback';
import SafeAreaConsumer from '@components/SafeAreaConsumer';
import SectionList from '@components/SectionList';
import Text from '@components/Text';
Expand All @@ -30,6 +29,7 @@ function BaseSelectionList<TItem extends ListItem>(
ListItem,
canSelectMultiple = false,
onSelectRow,
onCheckboxPress,
onSelectAll,
onDismissError,
textInputLabel = '',
Expand Down Expand Up @@ -289,6 +289,7 @@ function BaseSelectionList<TItem extends ListItem>(
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={() => selectRow(item)}
onCheckboxPress={onCheckboxPress ? () => onCheckboxPress?.(item) : undefined}
Copy link
Contributor

@situchan situchan Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

NAB:

Suggested change
onCheckboxPress={onCheckboxPress ? () => onCheckboxPress?.(item) : undefined}
onCheckboxPress={onCheckboxPress ? () => onCheckboxPress(item) : undefined}

onDismissError={() => onDismissError?.(item)}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
rightHandSideComponent={rightHandSideComponent}
Expand Down Expand Up @@ -429,28 +430,19 @@ function BaseSelectionList<TItem extends ListItem>(
) : (
<>
{!headerMessage && canSelectMultiple && shouldShowSelectAll && (
<PressableWithFeedback
style={[styles.peopleRow, styles.userSelectNone, styles.ph4, styles.pb3, listHeaderWrapperStyle]}
onPress={selectAllRow}
accessibilityLabel={translate('workspace.people.selectAll')}
role="button"
accessibilityState={{checked: flattenedSections.allSelected}}
disabled={flattenedSections.allOptions.length === flattenedSections.disabledOptionsIndexes.length}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
onMouseDown={shouldPreventDefaultFocusOnSelectRow ? (e) => e.preventDefault() : undefined}
>
<View style={[styles.peopleRow, styles.userSelectNone, styles.ph4, styles.pb3, listHeaderWrapperStyle]}>
<Checkbox
accessibilityLabel={translate('workspace.people.selectAll')}
isChecked={flattenedSections.allSelected}
onPress={selectAllRow}
disabled={flattenedSections.allOptions.length === flattenedSections.disabledOptionsIndexes.length}
accessibilityLabel={translate('workspace.people.selectAll')}
/>
{customListHeader ?? (
<View style={[styles.flex1]}>
<Text style={[styles.textStrong, styles.ph3]}>{translate('workspace.people.selectAll')}</Text>
</View>
)}
</PressableWithFeedback>
</View>
)}
{!headerMessage && !canSelectMultiple && customListHeader}
<SectionList
Expand Down
2 changes: 2 additions & 0 deletions src/components/SelectionList/RadioListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ function RadioListItem({
isDisabled,
canSelectMultiple,
onSelectRow,
onCheckboxPress,
onDismissError,
shouldPreventDefaultFocusOnSelectRow,
rightHandSideComponent,
Expand All @@ -30,6 +31,7 @@ function RadioListItem({
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={onSelectRow}
onCheckboxPress={onCheckboxPress}
onDismissError={onDismissError}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
rightHandSideComponent={rightHandSideComponent}
Expand Down
2 changes: 2 additions & 0 deletions src/components/SelectionList/TableListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ function TableListItem({
isDisabled,
canSelectMultiple,
onSelectRow,
onCheckboxPress,
onDismissError,
shouldPreventDefaultFocusOnSelectRow,
rightHandSideComponent,
Expand All @@ -37,6 +38,7 @@ function TableListItem({
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={onSelectRow}
onCheckboxPress={onCheckboxPress}
onDismissError={onDismissError}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
rightHandSideComponent={rightHandSideComponent}
Expand Down
2 changes: 2 additions & 0 deletions src/components/SelectionList/UserListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ function UserListItem({
isDisabled,
canSelectMultiple,
onSelectRow,
onCheckboxPress,
onDismissError,
shouldPreventDefaultFocusOnSelectRow,
rightHandSideComponent,
Expand All @@ -41,6 +42,7 @@ function UserListItem({
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={onSelectRow}
onCheckboxPress={onCheckboxPress}
onDismissError={onDismissError}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
rightHandSideComponent={rightHandSideComponent}
Expand Down
6 changes: 6 additions & 0 deletions src/components/SelectionList/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ type CommonListItemProps<TItem> = {
/** Callback to fire when the item is pressed */
onSelectRow: (item: TItem) => void;

/** Callback to fire when a checkbox is pressed */
onCheckboxPress?: (item: TItem) => void;

/** Callback to fire when an error is dismissed */
onDismissError?: (item: TItem) => void;

Expand Down Expand Up @@ -157,6 +160,9 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
/** Callback to fire when a row is pressed */
onSelectRow: (item: TItem) => void;

/** Optional callback function triggered upon pressing a checkbox. If undefined and the list displays checkboxes, checkbox interactions are managed by onSelectRow, allowing for pressing anywhere on the list. */
onCheckboxPress?: (item: TItem) => void;

/** Callback to fire when "Select All" checkbox is pressed. Only use along with `canSelectMultiple` */
onSelectAll?: () => void;

Expand Down
3 changes: 2 additions & 1 deletion src/pages/workspace/categories/WorkspaceCategoriesPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ function WorkspaceCategoriesPage({policyCategories, route}: WorkspaceCategoriesP
<SelectionList
canSelectMultiple
sections={[{data: categoryList, indexOffset: 0, isDisabled: false}]}
onSelectRow={toggleCategory}
onCheckboxPress={toggleCategory}
onSelectRow={navigateToCategorySettings}
onSelectAll={toggleAllCategories}
showScrollIndicator
ListItem={TableListItem}
Expand Down
Loading