-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Replace OptionsSelector with SelectionList - part 1 #38994
Replace OptionsSelector with SelectionList - part 1 #38994
Conversation
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.
Code looks good! We have to test these components thoroughly. Thanks for the extensive QA steps 🙌
@Santhosh-Sellavel 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] |
@Santhosh-Sellavel kind bump here :) |
Taking this as C+ from @Santhosh-Sellavel. I will review |
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.
Partial review
On report dropdown searching for a selected option should appear as selected Screen.Recording.2024-03-31.at.2.44.04.AM.mov |
// @ts-expect-error TODO: remove this comment once OptionsSelector (https://github.com/Expensify/App/issues/25125) is migrated to TS | ||
placeholder={translate('workspace.switcher.placeholder')} | ||
ref={inputCallbackRef} | ||
<SelectionList |
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.
There is still a everythingSection
that is using OptionRow. Can we move it to the passed sections
in SelectionList
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.
Hmm, it needs a huge refactor of the whole page, not sure if this is the right issue to do it. Btw. OptionRow
is also used in some other components and I don't know of we want to remove it with OptionsSelector
@s77rt All done! Left one thought |
@s77rt Ready for re-review! |
Reviewer Checklist
Screenshots/Videos |
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25125 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
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.
Lots of changes going on on this PR. I would recommend we do 1 page per PR next time. Thanks @s77rt for the tests.
I think this PR introduced this error. It happens both on Share logs and when you click on the workspace switcher. |
I'm reverting this PR so we can fix the error log that is happening before we merge it again. I recommend we do it step by step this time. We've checked the |
@@ -81,6 +81,14 @@ function BaseListItem<TItem extends ListItem>({ | |||
</View> | |||
</View> | |||
)} | |||
{!item.isSelected && item.brickRoadIndicator && ( |
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.
{!item.isSelected && !!item.brickRoadIndicator && (
|
||
{/* If we are loading new options we will avoid showing any header message. This is mostly because one of the header messages says there are no options. */} | ||
{/* This is misleading because we might be in the process of loading fresh options from the server. */} | ||
{!isLoadingNewOptions && headerMessage && ( |
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.
{!isLoadingNewOptions && !!headerMessage && (
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.
@danieldoglas Sorry I missed that. I added required change. cc @filip-solecki |
As for the duplicated variable I don't think we could have prevented that as it's a post-merge error |
I've prepared draft PR with fix, not sure if we should prepare these pages as separated PRs right now, we can do it with the rest of pages I think. I am checking new branch on all devices, I'll let you know @s77rt when it is ready for you! |
🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.4.61-0 🚀
|
🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.4.61-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.61-8 🚀
|
Details
First part of migration
OptionsSelector
toSelectionList
covers 4 pages. Remaining 3 will be covered in separate PR andOptionsSelector
will be removed there as well.Fixed Issues
$ #25125
PROPOSAL:
Tests
BaseShareLogList:
WorkspaceSwitcherPage
TagPicker
EditReportFieldDropdownPage
canUseReportFields
beta is enabled.Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
WorkspaceSwitcher
Android: Native
WorkspaceSwitcher_ANDROID.mov
Android: mWeb Chrome
WorkspaceSwitcher_AND_CHROME.mov
iOS: Native
WorkspaceSwitcher_IOS.mp4
iOS: mWeb Safari
WorkspaceSwitcher_IOS_SAFARI.mp4
MacOS: Chrome / Safari
Workspace_Switcher_WEB.mov
MacOS: Desktop
WorkspaceSwithcer_DESKTOP.mov
ShareLog
Android: Native
ShareLog_ANDROID.mov
Android: mWeb Chrome
ShareLog_AND_CHROME.mov
iOS: Native
ShareLog_IOS.mp4
iOS: mWeb Safari
ShareLog_IOS_SAFARI.mp4
MacOS: Chrome / Safari
Share_log_WEB.mov
MacOS: Desktop
ShareLog_DESKTOP.mov
TagPicker
Android: Native
TagPicker_ANDROID.mov
Android: mWeb Chrome
TagPicker_AND_CHROME.mov
iOS: Native
TagPicker_IOS.mp4
iOS: mWeb Safari
TagPicker_IOS_SAFARI.mp4
MacOS: Chrome / Safari
TagPicker_WEB.mov
MacOS: Desktop
TagPicker_DESKTOP.mov
EditReportFieldDropdownPage
Android: Native
Dropdown_ANDROID.mov
Android: mWeb Chrome
Dropdown_AND_CHROME.mov
iOS: Native
Dropdown_IOS.mp4
iOS: mWeb Safari
Dropdown_IOS_SAFARI.mp4
MacOS: Chrome / Safari
EditReportFieldDropdownPage_WEB.mov
MacOS: Desktop
Dropdown_DESKTOP.mov
EDIT Added `isSelected` mark in `EditReportFieldDropdownPage`, as on the screenshot below: