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

Always scroll to top when selecting item #56098

Merged
merged 5 commits into from
Feb 10, 2025
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
3 changes: 3 additions & 0 deletions __mocks__/@react-navigation/native/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ const useLinkTo = isJestEnv ? realReactNavigation.useLinkTo : () => null;
const useScrollToTop = isJestEnv ? realReactNavigation.useScrollToTop : () => null;
const useRoute = isJestEnv ? realReactNavigation.useRoute : () => ({params: {}});
const useFocusEffect = isJestEnv ? realReactNavigation.useFocusEffect : (callback: () => void) => callback();
// eslint-disable-next-line @typescript-eslint/naming-convention
const UNSTABLE_usePreventRemove = isJestEnv ? jest.fn() : () => {};

export {
// Overriden modules
Expand All @@ -66,6 +68,7 @@ export {
useScrollToTop,
useRoute,
useFocusEffect,
UNSTABLE_usePreventRemove,
};

export type {NativeNavigationMock};
14 changes: 3 additions & 11 deletions src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -382,16 +382,9 @@ function BaseSelectionList<TItem extends ListItem>(
(item: TItem, indexToFocus?: number) => {
// In single-selection lists we don't care about updating the focused index, because the list is closed after selecting an item
if (canSelectMultiple) {
if (sections.length > 1) {
// If the list has only 1 section (e.g. Workspace Members list), we do nothing.
// If the list has multiple sections (e.g. Workspace Invite list), and `shouldUnfocusRow` is false,
// we focus the first one after all the selected (selected items are always at the top).
const selectedOptionsCount = item.isSelected ? flattenedSections.selectedOptions.length - 1 : flattenedSections.selectedOptions.length + 1;

if (!item.isSelected) {
// If we're selecting an item, scroll to it's position at the top, so we can see it
scrollToIndex(Math.max(selectedOptionsCount - 1, 0), true);
}
if (sections.length > 1 && !item.isSelected) {
// If we're selecting an item, scroll to it's position at the top, so we can see it
scrollToIndex(0, true);
}

if (shouldShowTextInput) {
Expand All @@ -412,7 +405,6 @@ function BaseSelectionList<TItem extends ListItem>(
[
canSelectMultiple,
sections.length,
flattenedSections.selectedOptions.length,
scrollToIndex,
shouldShowTextInput,
clearInputAfterSelect,
Expand Down
2 changes: 1 addition & 1 deletion src/pages/NewChatPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,11 @@ function NewChatPage() {
newSelectedOptions = reject(selectedOptions, (selectedOption) => selectedOption.login === option.login);
} else {
newSelectedOptions = [...selectedOptions, {...option, isSelected: true, selected: true, reportID: option.reportID ?? `${CONST.DEFAULT_NUMBER_ID}`}];
selectionListRef?.current?.scrollToIndex(0, true);
}

selectionListRef?.current?.clearInputAfterSelect?.();
selectionListRef.current?.focusTextInput();
selectionListRef?.current?.scrollToIndex(Math.max(newSelectedOptions.length - 1, 0), true);
setSelectedOptions(newSelectedOptions);
},
[selectedOptions, setSelectedOptions],
Expand Down
52 changes: 52 additions & 0 deletions tests/ui/NewChatPageTest.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import * as NativeNavigation from '@react-navigation/native';
import {act, fireEvent, render, screen} from '@testing-library/react-native';
import React from 'react';
import {SectionList} from 'react-native';
import Onyx from 'react-native-onyx';
import {LocaleContextProvider} from '@components/LocaleContextProvider';
import OnyxProvider from '@components/OnyxProvider';
import OptionsListContextProvider from '@components/OptionListContextProvider';
import ScreenWrapper from '@components/ScreenWrapper';
import {translateLocal} from '@libs/Localize';
import NewChatPage from '@pages/NewChatPage';
import ONYXKEYS from '@src/ONYXKEYS';
import type {NativeNavigationMock} from '../../__mocks__/@react-navigation/native';
import {fakePersonalDetails} from '../utils/LHNTestUtils';
import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct';

jest.mock('@react-navigation/native');

describe('NewChatPage', () => {
beforeAll(() => {
Onyx.init({
keys: ONYXKEYS,
});
jest.spyOn(NativeNavigation, 'useRoute').mockReturnValue({key: '', name: ''});
});

it('should scroll to top when adding a user to the group selection', async () => {
await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, fakePersonalDetails);
render(
<OnyxProvider>
<LocaleContextProvider>
<OptionsListContextProvider>
<ScreenWrapper testID="test">
<NewChatPage />
</ScreenWrapper>
</OptionsListContextProvider>
</LocaleContextProvider>
</OnyxProvider>,
);
await waitForBatchedUpdatesWithAct();
await act(() => {
(NativeNavigation as NativeNavigationMock).triggerTransitionEnd();
});
const spy = jest.spyOn(SectionList.prototype, 'scrollToLocation');

const addButton = screen.getAllByText(translateLocal('newChatPage.addToGroup')).at(0);
if (addButton) {
fireEvent.press(addButton);
expect(spy).toHaveBeenCalledWith(expect.objectContaining({itemIndex: 0}));
}
});
});
31 changes: 23 additions & 8 deletions tests/unit/BaseSelectionListTest.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import {fireEvent, render, screen} from '@testing-library/react-native';
import {SectionList} from 'react-native';
import BaseSelectionList from '@components/SelectionList/BaseSelectionList';
import RadioListItem from '@components/SelectionList/RadioListItem';
import type {ListItem} from '@components/SelectionList/types';
import type {BaseSelectionListProps, ListItem} from '@components/SelectionList/types';
import type Navigation from '@libs/Navigation/Navigation';
import CONST from '@src/CONST';

type BaseSelectionListSections<TItem extends ListItem> = {
sections: TItem[];
sections: BaseSelectionListProps<TItem>['sections'];
canSelectMultiple?: boolean;
};

const mockSections = Array.from({length: 10}, (_, index) => ({
Expand All @@ -28,21 +30,22 @@ describe('BaseSelectionList', () => {
const onSelectRowMock = jest.fn();

function BaseListItemRenderer<TItem extends ListItem>(props: BaseSelectionListSections<TItem>) {
const {sections} = props;
const focusedKey = sections.find((item) => item.isSelected)?.keyForList;
const {sections, canSelectMultiple} = props;
const focusedKey = sections[0].data.find((item) => item.isSelected)?.keyForList;
return (
<BaseSelectionList
sections={[{data: sections}]}
sections={sections}
ListItem={RadioListItem}
onSelectRow={onSelectRowMock}
shouldSingleExecuteRowSelect
canSelectMultiple={canSelectMultiple}
initiallyFocusedOptionKey={focusedKey}
/>
);
}

it('should handle item press correctly', () => {
render(<BaseListItemRenderer sections={mockSections} />);
render(<BaseListItemRenderer sections={[{data: mockSections}]} />);
fireEvent.press(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}1`));
expect(onSelectRowMock).toHaveBeenCalledWith({
...mockSections.at(1),
Expand All @@ -55,9 +58,21 @@ describe('BaseSelectionList', () => {
...section,
isSelected: section.keyForList === '2',
}));
const {rerender} = render(<BaseListItemRenderer sections={mockSections} />);
const {rerender} = render(<BaseListItemRenderer sections={[{data: mockSections}]} />);
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}1`)).toBeSelected();
rerender(<BaseListItemRenderer sections={updatedMockSections} />);
rerender(<BaseListItemRenderer sections={[{data: updatedMockSections}]} />);
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}2`)).toBeSelected();
});

it('should scroll to top when selecting a multi option list', () => {
const spy = jest.spyOn(SectionList.prototype, 'scrollToLocation');
render(
<BaseListItemRenderer
sections={[{data: []}, {data: mockSections}]}
canSelectMultiple
/>,
);
fireEvent.press(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}0`));
expect(spy).toHaveBeenCalledWith(expect.objectContaining({itemIndex: 0}));
});
});