From 689c8d859d8d75b561bd2e9f62711e8b9dfafca8 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 31 Jan 2025 01:17:15 +0800 Subject: [PATCH 1/3] always scroll to top when selectin item --- __mocks__/@react-navigation/native/index.ts | 4 +- .../SelectionList/BaseSelectionList.tsx | 13 +---- src/pages/NewChatPage.tsx | 2 +- tests/ui/NewChatPageTest.tsx | 57 +++++++++++++++++++ tests/unit/BaseSelectionListTest.tsx | 31 +++++++--- 5 files changed, 87 insertions(+), 20 deletions(-) create mode 100644 tests/ui/NewChatPageTest.tsx diff --git a/__mocks__/@react-navigation/native/index.ts b/__mocks__/@react-navigation/native/index.ts index 55d19124e65e..de1e8908734b 100644 --- a/__mocks__/@react-navigation/native/index.ts +++ b/__mocks__/@react-navigation/native/index.ts @@ -42,8 +42,9 @@ const useLinkBuilder = isJestEnv ? realReactNavigation.useLinkBuilder : () => nu const useLinkProps = isJestEnv ? realReactNavigation.useLinkProps : () => null; const useLinkTo = isJestEnv ? realReactNavigation.useLinkTo : () => null; const useScrollToTop = isJestEnv ? realReactNavigation.useScrollToTop : () => null; -const useRoute = isJestEnv ? realReactNavigation.useRoute : () => ({params: {}}); +const useRoute = isJestEnv ? jest.fn() : () => ({params: {}}); const useFocusEffect = isJestEnv ? realReactNavigation.useFocusEffect : (callback: () => void) => callback(); +const UNSTABLE_usePreventRemove = isJestEnv ? jest.fn() : () => {}; export { // Overriden modules @@ -66,6 +67,7 @@ export { useScrollToTop, useRoute, useFocusEffect, + UNSTABLE_usePreventRemove, }; export type {NativeNavigationMock}; diff --git a/src/components/SelectionList/BaseSelectionList.tsx b/src/components/SelectionList/BaseSelectionList.tsx index 86a9d8adbd76..567c71facf3f 100644 --- a/src/components/SelectionList/BaseSelectionList.tsx +++ b/src/components/SelectionList/BaseSelectionList.tsx @@ -363,16 +363,9 @@ function BaseSelectionList( (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) { diff --git a/src/pages/NewChatPage.tsx b/src/pages/NewChatPage.tsx index e2df8ceda5c9..a2a47c44b2c1 100755 --- a/src/pages/NewChatPage.tsx +++ b/src/pages/NewChatPage.tsx @@ -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], diff --git a/tests/ui/NewChatPageTest.tsx b/tests/ui/NewChatPageTest.tsx new file mode 100644 index 000000000000..7d04179b0f09 --- /dev/null +++ b/tests/ui/NewChatPageTest.tsx @@ -0,0 +1,57 @@ +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 createPersonalDetails from '../utils/collections/personalDetails'; +import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; + +jest.mock('@react-navigation/native'); + +describe('NewChatPage', () => { + beforeAll(() => { + Onyx.init({ + keys: ONYXKEYS, + }); + }); + + it('should scroll to top when adding a user to the group selection', async () => { + await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, { + 1: createPersonalDetails(1), + 2: createPersonalDetails(2), + 3: createPersonalDetails(3), + 4: createPersonalDetails(4), + 5: createPersonalDetails(5), + }); + render( + + + + + + + + + , + ); + 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})); + } + }); +}); diff --git a/tests/unit/BaseSelectionListTest.tsx b/tests/unit/BaseSelectionListTest.tsx index 673ea50b58d6..e52173d794ff 100644 --- a/tests/unit/BaseSelectionListTest.tsx +++ b/tests/unit/BaseSelectionListTest.tsx @@ -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 = { - sections: TItem[]; + sections: BaseSelectionListProps['sections']; + canSelectMultiple?: boolean; }; const mockSections = Array.from({length: 10}, (_, index) => ({ @@ -28,21 +30,22 @@ describe('BaseSelectionList', () => { const onSelectRowMock = jest.fn(); function BaseListItemRenderer(props: BaseSelectionListSections) { - 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 ( ); } it('should handle item press correctly', () => { - render(); + render(); fireEvent.press(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}1`)); expect(onSelectRowMock).toHaveBeenCalledWith({ ...mockSections.at(1), @@ -55,9 +58,21 @@ describe('BaseSelectionList', () => { ...section, isSelected: section.keyForList === '2', })); - const {rerender} = render(); + const {rerender} = render(); expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}1`)).toBeSelected(); - rerender(); + rerender(); expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}2`)).toBeSelected(); }); + + it('should scroll to top when selecting a multi option list', async () => { + const spy = jest.spyOn(SectionList.prototype, 'scrollToLocation'); + render( + , + ); + fireEvent.press(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}0`)); + expect(spy).toHaveBeenCalledWith(expect.objectContaining({itemIndex: 0})); + }); }); From a984e7fe83d8340a60876769516a75d4bc4919e2 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Sat, 1 Feb 2025 00:57:46 +0800 Subject: [PATCH 2/3] linr --- __mocks__/@react-navigation/native/index.ts | 1 + src/components/SelectionList/BaseSelectionList.tsx | 1 - tests/ui/NewChatPageTest.tsx | 10 ++-------- tests/unit/BaseSelectionListTest.tsx | 2 +- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/__mocks__/@react-navigation/native/index.ts b/__mocks__/@react-navigation/native/index.ts index de1e8908734b..72f0a1fdf4d2 100644 --- a/__mocks__/@react-navigation/native/index.ts +++ b/__mocks__/@react-navigation/native/index.ts @@ -44,6 +44,7 @@ const useLinkTo = isJestEnv ? realReactNavigation.useLinkTo : () => null; const useScrollToTop = isJestEnv ? realReactNavigation.useScrollToTop : () => null; const useRoute = isJestEnv ? jest.fn() : () => ({params: {}}); const useFocusEffect = isJestEnv ? realReactNavigation.useFocusEffect : (callback: () => void) => callback(); +// eslint-disable-next-line @typescript-eslint/naming-convention const UNSTABLE_usePreventRemove = isJestEnv ? jest.fn() : () => {}; export { diff --git a/src/components/SelectionList/BaseSelectionList.tsx b/src/components/SelectionList/BaseSelectionList.tsx index 567c71facf3f..d3c9f19f5a48 100644 --- a/src/components/SelectionList/BaseSelectionList.tsx +++ b/src/components/SelectionList/BaseSelectionList.tsx @@ -386,7 +386,6 @@ function BaseSelectionList( [ canSelectMultiple, sections.length, - flattenedSections.selectedOptions.length, scrollToIndex, shouldShowTextInput, clearInputAfterSelect, diff --git a/tests/ui/NewChatPageTest.tsx b/tests/ui/NewChatPageTest.tsx index 7d04179b0f09..b3494d9eca53 100644 --- a/tests/ui/NewChatPageTest.tsx +++ b/tests/ui/NewChatPageTest.tsx @@ -11,7 +11,7 @@ import {translateLocal} from '@libs/Localize'; import NewChatPage from '@pages/NewChatPage'; import ONYXKEYS from '@src/ONYXKEYS'; import type {NativeNavigationMock} from '../../__mocks__/@react-navigation/native'; -import createPersonalDetails from '../utils/collections/personalDetails'; +import {fakePersonalDetails} from '../utils/LHNTestUtils'; import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; jest.mock('@react-navigation/native'); @@ -24,13 +24,7 @@ describe('NewChatPage', () => { }); it('should scroll to top when adding a user to the group selection', async () => { - await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, { - 1: createPersonalDetails(1), - 2: createPersonalDetails(2), - 3: createPersonalDetails(3), - 4: createPersonalDetails(4), - 5: createPersonalDetails(5), - }); + await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, fakePersonalDetails); render( diff --git a/tests/unit/BaseSelectionListTest.tsx b/tests/unit/BaseSelectionListTest.tsx index e52173d794ff..07ac5f4667ca 100644 --- a/tests/unit/BaseSelectionListTest.tsx +++ b/tests/unit/BaseSelectionListTest.tsx @@ -64,7 +64,7 @@ describe('BaseSelectionList', () => { expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}2`)).toBeSelected(); }); - it('should scroll to top when selecting a multi option list', async () => { + it('should scroll to top when selecting a multi option list', () => { const spy = jest.spyOn(SectionList.prototype, 'scrollToLocation'); render( Date: Mon, 3 Feb 2025 00:00:32 +0800 Subject: [PATCH 3/3] fix test --- __mocks__/@react-navigation/native/index.ts | 2 +- tests/ui/NewChatPageTest.tsx | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/__mocks__/@react-navigation/native/index.ts b/__mocks__/@react-navigation/native/index.ts index 72f0a1fdf4d2..9cd0e497d75e 100644 --- a/__mocks__/@react-navigation/native/index.ts +++ b/__mocks__/@react-navigation/native/index.ts @@ -42,7 +42,7 @@ const useLinkBuilder = isJestEnv ? realReactNavigation.useLinkBuilder : () => nu const useLinkProps = isJestEnv ? realReactNavigation.useLinkProps : () => null; const useLinkTo = isJestEnv ? realReactNavigation.useLinkTo : () => null; const useScrollToTop = isJestEnv ? realReactNavigation.useScrollToTop : () => null; -const useRoute = isJestEnv ? jest.fn() : () => ({params: {}}); +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() : () => {}; diff --git a/tests/ui/NewChatPageTest.tsx b/tests/ui/NewChatPageTest.tsx index b3494d9eca53..837f59bf3af4 100644 --- a/tests/ui/NewChatPageTest.tsx +++ b/tests/ui/NewChatPageTest.tsx @@ -21,6 +21,7 @@ describe('NewChatPage', () => { 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 () => {