-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Stop scrolling to top when selecting an item #65449
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
Stop scrolling to top when selecting an item #65449
Conversation
|
When we pass the first index using
What is the expected behavior for the highlight for both single selection list and multi selection list? Screen.Recording.2025-07-04.at.12.54.14.AM.movBasically after applying following diff suggested in original proposal--- a/src/components/Search/SearchMultipleSelectionPicker.tsx
+++ b/src/components/Search/SearchMultipleSelectionPicker.tsx
@@ -8,6 +8,7 @@ import type {OptionData} from '@libs/ReportUtils';
import {sortOptionsWithEmptyValue} from '@libs/SearchQueryUtils';
import ROUTES from '@src/ROUTES';
import SearchFilterPageFooterButtons from './SearchFilterPageFooterButtons';
+import SelectableListItem from '@components/SelectionList/SelectableListItem';
type SearchMultipleSelectionPickerItem = {
name: string;
@@ -32,35 +33,26 @@ function SearchMultipleSelectionPicker({items, initiallySelectedItem
s, pickerTit
setSelectedItems(initiallySelectedItems ?? []);
}, [initiallySelectedItems]);
- const {sections, noResultsFound} = useMemo(() => {
- const selectedItemsSection = selectedItems
- .filter((item) => item?.name.toLowerCase().includes(debouncedSearchTerm?.to
LowerCase()))
- .sort((a, b) => sortOptionsWithEmptyValue(a.value as string, b.value as str
ing))
- .map((item) => ({
- text: item.name,
- keyForList: item.name,
- isSelected: true,
- value: item.value,
- }));
+ const {sections, noResultsFound, firstKeyForList} = useMemo(() => {
const remainingItemsSection = items
- .filter((item) => selectedItems.some((selectedItem) => selectedItem.value =
== item.value) === false && item?.name?.toLowerCase().includes(debouncedSearchTerm?.toLo
werCase()))
- .sort((a, b) => sortOptionsWithEmptyValue(a.value as string, b.value as str
ing))
+ .filter((item) => item?.name?.toLowerCase().includes(debouncedSearchTerm?.t
oLowerCase()))
+ .sort((a, b) => sortOptionsWithEmptyValue(a, b))
.map((item) => ({
text: item.name,
keyForList: item.name,
- isSelected: false,
+ isSelected: selectedItems.some((selectedItem) => selectedItem.value ===
item.value),
value: item.value,
}));
- const isEmpty = !selectedItemsSection.length && !remainingItemsSection.length;
+
+ const isEmpty = !remainingItemsSection.length;
+
+ const firstSelectedItem = remainingItemsSection.find((item) => item.isSelected)
;
+ const firstKey = firstSelectedItem?.keyForList ?? null;
+
return {
sections: isEmpty
? []
: [
- {
- title: undefined,
- data: selectedItemsSection,
- shouldShow: selectedItemsSection.length > 0,
- },
{
title: pickerTitle,
data: remainingItemsSection,
@@ -68,6 +60,7 @@ function SearchMultipleSelectionPicker({items, initiallySelectedItems,
pickerTit
},
],
noResultsFound: isEmpty,
+ firstKeyForList: firstKey,
};
}, [selectedItems, items, pickerTitle, debouncedSearchTerm]);
@@ -116,7 +109,8 @@ function SearchMultipleSelectionPicker({items, initiallySelectedItem
s, pickerTit
showLoadingPlaceholder={!noResultsFound}
shouldShowTooltips
canSelectMultiple
- ListItem={MultiSelectListItem}
+ ListItem={SelectableListItem}
+ initiallyFocusedOptionKey={firstKeyForList}
/>
);
} |
|
@parasharrajat waiting for your reply to proceed further... |
|
Let's update this behaviour in a way so that it does not highlight. We are only using this prop to scroll to the item. |
|
For a single selection list, Should we highlight? |
|
When the user is using arrow keys to navigate, it makes sense to have highlight to show the active item in the list but otherwise, I don't the point of highlight. @shawnborton Can you please help us determine whether we should highlight the row or not? |
|
Hmm why would we change any of the current arrow behavior here? We should just continue to use the existing arrow highlight behavior that we currently have, no? cc @Expensify/design for a gut check. |
|
I am not asking about the arrows, I am asking about background colour |
|
Yeah same thing, why would we change any of that behavior? We should leave it as-is. |
|
Agree with Shawn, I don't see why we'd need to mess with any of that here. |
c5fb170 to
1bf1112
Compare
|
@parasharrajat are we looking to apply these changes to popover filters as well? Screen.Recording.2025-07-23.at.10.08.21.PM.mov |
|
Let's not do that for now. If needed we can do that in separate PR. |
|
Actually |
|
Let's keep such pages aside for now. Focus on other SelectionList-based logic. Once you are done, you can list down such pages for us and then we can tackle then in separate PR. |
|
@ChavdaSachin What's latest here? |
|
Most of the pages which are yet to be fixed are related to member lists, and since member lists have quite a complex structure it is taking some time. |
|
If you don't have time to work on this. Let me us know the progress and pending items so we can decide on next steps here. |
|
Note: Reason: Split expense flow has been removed from the app, so it is not possible for value of all the other pages where cc. @parasharrajat ps. Contributor who worked on removing split expense flow should refactor all such pages. |
|
Tests: Search page:
Group:
Room:
Workspace: (create one if needed)
IOU: (prerequisite: user has a control workspace set as the default workspace)
Onboarding:
|
This comment was marked as resolved.
This comment was marked as resolved.
…lling-and-jumping-selectionList
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@parasharrajat 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] |
|
@ChavdaSachin Can you please merge main? |
|
Sure thing, I am ooo today and tomorrow, I should be able to work it out on Tuesday. |
|
@parasharrajat I could see there are far too many changes which could be resolved here. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the product changes happening in this PR.
|
@ChavdaSachin Looks like the PR you tagged is merged. What is your new plan? |
Explanation of Change
Fixed Issues
$ #61414
PROPOSAL: #61414 (comment)
Tests
Same as QA.
Offline tests
Same as QA.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Tests:
Search page:
Type> selectExpenseand save.Fromfilter.saveTo,Workspace,Card,Category,Currency,TagandTax Raterepeat steps 4-10.Type> selectChatand save.Inrepeat steps 4-10.Type> selectTaskand save.Assigneerepeat steps 4-10.Group:
start chat.Add to groupbutton.Next>Start Group.Members>Invite members.Room:
Start chat>Room> create a room.Members>Invite member.Workspace: (create one if needed)
Workspaces> open a workspace.Members>Invite Member.IOU: (prerequisite: user has a control workspace set as the default workspace)
Create expense>Manual.Next.Attendees.Save.Attendees.Onboarding:
Work Emailstep.Track and budget expenseson purpose select step.First NameandLast Name, clickcontinue.Create workspace>Continue.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Screen.Recording.2025-09-02.at.12.27.48.PM.mov
Screen.Recording.2025-09-02.at.12.33.45.PM.mov
Screen.Recording.2025-09-02.at.12.35.00.PM.mov
Screen.Recording.2025-09-02.at.12.36.03.PM.mov
Screen.Recording.2025-09-02.at.12.36.42.PM.mov
Screen.Recording.2025-09-02.at.12.40.49.PM.mov
Android: mWeb Chrome
Screen.Recording.2025-09-02.at.12.47.53.PM.mov
Screen.Recording.2025-09-02.at.12.50.56.PM.mov
Screen.Recording.2025-09-02.at.12.51.41.PM.mov
Screen.Recording.2025-09-02.at.12.52.44.PM.mov
Screen.Recording.2025-09-02.at.12.53.13.PM.mov
Screen.Recording.2025-09-02.at.12.55.05.PM.mov
iOS: Native
Screen.Recording.2025-09-02.at.11.40.25.AM.mov
Screen.Recording.2025-09-02.at.11.46.32.AM.mov
Screen.Recording.2025-09-02.at.11.47.35.AM.mov
Screen.Recording.2025-09-02.at.11.48.58.AM.mov
Screen.Recording.2025-09-02.at.11.51.02.AM.mov
Screen.Recording.2025-09-02.at.11.57.32.AM.mov
iOS: mWeb Safari
Screen.Recording.2025-09-02.at.12.03.21.PM.mov
Screen.Recording.2025-09-02.at.12.07.41.PM.mov
Screen.Recording.2025-09-02.at.12.09.47.PM.mov
Screen.Recording.2025-09-02.at.12.11.30.PM.mov
Screen.Recording.2025-09-02.at.12.12.05.PM.mov
Screen.Recording.2025-09-02.at.12.16.08.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2025-08-31.at.7.03.14.PM.mov
Screen.Recording.2025-08-31.at.7.06.01.PM.mov
Screen.Recording.2025-08-31.at.7.07.00.PM.mov
Screen.Recording.2025-08-31.at.7.07.44.PM.mov
Screen.Recording.2025-08-31.at.7.08.24.PM.mov
Screen.Recording.2025-08-31.at.7.10.55.PM.mov
MacOS: Desktop
Screen.Recording.2025-09-02.at.1.17.37.PM.mov
Screen.Recording.2025-09-02.at.1.21.15.PM.mov
Screen.Recording.2025-09-02.at.1.23.15.PM.mov
Screen.Recording.2025-09-02.at.1.23.44.PM.mov
Screen.Recording.2025-09-02.at.1.24.13.PM.mov
Screen.Recording.2025-09-02.at.1.24.50.PM.mov