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

Fix can't navigate with arrow key to admin in group members page #41968

Merged
Show file tree
Hide file tree
Changes from all 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
23 changes: 15 additions & 8 deletions src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -103,16 +103,18 @@ function BaseSelectionList<TItem extends ListItem>(
const incrementPage = () => setCurrentPage((prev) => prev + 1);

/**
* Iterates through the sections and items inside each section, and builds 3 arrays along the way:
* Iterates through the sections and items inside each section, and builds 4 arrays along the way:
* - `allOptions`: Contains all the items in the list, flattened, regardless of section
* - `disabledOptionsIndexes`: Contains the indexes of all the disabled items in the list, to be used by the ArrowKeyFocusManager
* - `disabledOptionsIndexes`: Contains the indexes of all the unselectable and disabled items in the list
* - `disabledArrowKeyOptionsIndexes`: Contains the indexes of item that is not navigatable by the arrow key. The list is separated from disabledOptionsIndexes because unselectable item is still navigatable by the arrow key.
* - `itemLayouts`: Contains the layout information for each item, header and footer in the list,
* so we can calculate the position of any given item when scrolling programmatically
*/
const flattenedSections = useMemo<FlattenedSectionsReturn<TItem>>(() => {
const allOptions: TItem[] = [];

const disabledOptionsIndexes: number[] = [];
const disabledArrowKeyOptionsIndexes: number[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment explaining what these two arrays are for? Or add it to the comment above the flattenedSections function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding. It's not super obvious to me how disabledOptionsIndexes and disabledArrowKeyOptionsIndexes are different. Can we try to make that a little bit more explicit and obvious?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conflicts solved and comment updated. Please check

let disabledIndex = 0;

let offset = 0;
Expand All @@ -134,9 +136,13 @@ function BaseSelectionList<TItem extends ListItem>(
});

// If disabled, add to the disabled indexes array
const isItemDisabled = !!section.isDisabled || (item.isDisabled && !item.isSelected);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
if (!!section.isDisabled || (item.isDisabled && !item.isSelected) || item.isDisabledCheckbox) {
if (isItemDisabled || item.isDisabledCheckbox) {
disabledOptionsIndexes.push(disabledIndex);
if (isItemDisabled) {
disabledArrowKeyOptionsIndexes.push(disabledIndex);
}
}
disabledIndex += 1;

Expand Down Expand Up @@ -169,6 +175,7 @@ function BaseSelectionList<TItem extends ListItem>(
allOptions,
selectedOptions,
disabledOptionsIndexes,
disabledArrowKeyOptionsIndexes,
itemLayouts,
allSelected: selectedOptions.length > 0 && selectedOptions.length === allOptions.length - disabledOptionsIndexes.length,
};
Expand Down Expand Up @@ -230,21 +237,21 @@ function BaseSelectionList<TItem extends ListItem>(
[flattenedSections.allOptions],
);

const [disabledIndexes, setDisabledIndexes] = useState(flattenedSections.disabledOptionsIndexes);
const [disabledArrowKeyIndexes, setDisabledArrowKeyIndexes] = useState(flattenedSections.disabledArrowKeyOptionsIndexes);
useEffect(() => {
if (arraysEqual(disabledIndexes, flattenedSections.disabledOptionsIndexes)) {
if (arraysEqual(disabledArrowKeyIndexes, flattenedSections.disabledArrowKeyOptionsIndexes)) {
return;
}

setDisabledIndexes(flattenedSections.disabledOptionsIndexes);
setDisabledArrowKeyIndexes(flattenedSections.disabledArrowKeyOptionsIndexes);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [flattenedSections.disabledOptionsIndexes]);
}, [flattenedSections.disabledArrowKeyOptionsIndexes]);

// If `initiallyFocusedOptionKey` is not passed, we fall back to `-1`, to avoid showing the highlight on the first member
const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({
initialFocusedIndex: flattenedSections.allOptions.findIndex((option) => option.keyForList === initiallyFocusedOptionKey),
maxIndex: Math.min(flattenedSections.allOptions.length - 1, CONST.MAX_SELECTION_LIST_PAGE_LENGTH * currentPage - 1),
disabledIndexes,
disabledIndexes: disabledArrowKeyIndexes,
isActive: true,
onFocusedIndexChange: (index: number) => {
scrollToIndex(index, true);
Expand Down
1 change: 1 addition & 0 deletions src/components/SelectionList/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,7 @@ type FlattenedSectionsReturn<TItem extends ListItem> = {
allOptions: TItem[];
selectedOptions: TItem[];
disabledOptionsIndexes: number[];
disabledArrowKeyOptionsIndexes: number[];
itemLayouts: ItemLayout[];
allSelected: boolean;
};
Expand Down
Loading