fix: Tags - List jumps to “Show More” when selecting tag after 500.#66780
fix: Tags - List jumps to “Show More” when selecting tag after 500.#66780rafecolton merged 16 commits intoExpensify:mainfrom
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| } | ||
| // Reset the current page to 1 when the user types something | ||
| setCurrentPage(1); | ||
| onCurrentPageChange?.(); |
There was a problem hiding this comment.
Why do we need to add this callback?
There was a problem hiding this comment.
It's for the unit tests.
There was a problem hiding this comment.
It's not a good reason to add those callbacks if they're only used for testing purposes.
There was a problem hiding this comment.
I have seen that type of case before and also I don't think there's any other way to add tests. Do you think we should remove the tests?
There was a problem hiding this comment.
Is it possible to test behaviors over callback here? I mean when the current page is reset, we expect user won't see items on page 2.
| // Reset the current page to 1 when the user types something | ||
| setCurrentPage(1); | ||
|
|
||
| onFocusReset?.(); |
There was a problem hiding this comment.
Why do we need to have this?
There was a problem hiding this comment.
It's for the unit tests.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}50`)).toBeTruthy(); | ||
| expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}99`)).toBeTruthy(); | ||
|
|
||
| // Should not show, Show more button as we rendered whole list and search text was not changed |
There was a problem hiding this comment.
| // Should not show, Show more button as we rendered whole list and search text was not changed | |
| // Should not show "Show more" button as we rendered whole list and search text was not changed |
| rerender( | ||
| <BaseListItemRenderer | ||
| sections={[{data: largeMockSections.map((item, index) => ({...item, isSelected: index === 3}))}]} | ||
| canSelectMultiple={false} | ||
| initialNumToRender={110} | ||
| />, | ||
| ); |
There was a problem hiding this comment.
rerender to mock sections look here looks incorrect to me. Btw, we should simulate the behavior that user tap on the checkbox, not the "show more" button.
There was a problem hiding this comment.
Why does re-rendering seem incorrect to you? We just need to update the selected options array and ensure that the currentPage is not reset to 1. I think this is the correct way to handle it.
There was a problem hiding this comment.
I'm expecting we should simulate press checkbox like this
App/tests/unit/BaseSelectionListTest.tsx
Line 52 in 4b0bfd6
Re-render only makes sense if the props is updated from outside like this
App/tests/unit/BaseSelectionListTest.tsx
Lines 67 to 77 in 4b0bfd6
There was a problem hiding this comment.
From this recording #66780 (comment). Are you saying that we can't write test for this simulate press checkbox case?
There was a problem hiding this comment.
We can simulate a checkbox press, but it will have no effect because the options with property isSelected come from the parent component, which uses SelectionList. The selected options only change if the parent component handles the onSelectRow callback and provides updated sections to the SelectionList via sections={[{data: updatedLargeMockSections}]}.
it('does not reset page when only selectedOptions changes', async () => {
render(
<BaseListItemRenderer
sections={[{data: largeMockSections}]}
canSelectMultiple
initialNumToRender={600}
listItem={TableListItem}
/>,
);
await waitForBatchedUpdatesWithAct();
// Click Show More button
fireEvent.press(screen.getByText('common.showMore'));
fireEvent.press(screen.getByTestId(`TableListItemCheckbox-Item 598`));
expect(onSelectRowMock).toHaveBeenCalledTimes(1);
await waitForBatchedUpdatesWithAct();
// Should now show items from second page
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}500`)).toBeTruthy();
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}503`)).toBeTruthy();
// Should not show, "Show more" button as we rendered whole list and search text was not changed
expect(screen.queryByText('common.showMore')).toBeFalsy();
});
|
@hoangzinh, this PR is reverted again, should we wait? |
|
@Krishna2323 is there any reason why we have to wait? |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Never mind, I thought of something else. I think we should proceed without waiting for the other PR. I have resolved the merge conflicts. |
|
@Krishna2323 can you take a look at my comment here #66780 (comment)? Thank you |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@hoangzinh, on workspace list pages, the current page doesn’t reset to page 1 when the search input changes. However, on the participants page, it does reset to page 1 after a search. Should we fix this? I think no, because resetting makes sense when selected options appear at the top of the list (like on the participants page). But for workspace lists, we shouldn’t reset the page. What do you think? This doesn’t seem related to the bug we’re solving, but we should confirm and address it if necessary. PS: I updated cc: @Expensify/design Monosnap.screencast.2025-07-28.10-02-59.mp4 |
|
I agreed that we should leave it as it is until we have a real reported issue/bug with it. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-07-30.at.06.03.10.android.movAndroid: mWeb ChromeScreen.Recording.2025-07-30.at.05.38.53.android.chrome.moviOS: HybridAppScreen.Recording.2025-07-30.at.06.14.34.moviOS: mWeb SafariScreen.Recording.2025-07-30.at.06.06.25.movMacOS: Chrome / SafariScreen.Recording.2025-07-30.at.05.27.35.web.movMacOS: DesktopScreen.Recording.2025-07-30.at.05.32.55.desktop.mov |
|
@Krishna2323 @hoangzinh can you please re-test on main and see if this issue still occurs? I came to review this and noticed @mvtglobally's comment here that this is no longer reproducible 🤔 |
|
@rafecolton issue is still reproducible: Monosnap.screencast.2025-08-01.09-08-40.mp4 |
|
@Krishna2323 Can you please merge main and resolve conflicts? |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@rafecolton conflicts resolved. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/rafecolton in version: 9.1.90-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.90-11 🚀
|
4 participants
Explanation of Change
Fixed Issues
$ #65977
PROPOSAL: #65977 (comment)
Tests
Pre-condition: import tags using: Bug6885040_1752262047947!Over_500_tags (1).csv
Offline tests
Pre-condition: import tags using: Bug6885040_1752262047947!Over_500_tags (1).csv
QA Steps
Pre-condition: import tags using: Bug6885040_1752262047947!Over_500_tags (1).csv
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4