-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[WIP] Clean up BaseOptionsSelector #37353
Conversation
# Conflicts: # src/components/OptionsSelector/BaseOptionsSelector.js
adjustedSectionIndex part is not required inside the |
Going to close this as we're reverting the PR that changed this from a class component to a functional component, and we'll let the contributor who did that integrate these changes in their second try, when we're not so rushed. The main thing I'm aware of that's not working on this branch is the pagination functionality, but I'm not sure why. |
Actually I'm going to reopen this without the intention of having it merged to reflect further comments on the code. |
Can you pls mention where this is happening? PR!! |
Update: re-refactoring is ready to be tested, it fixes all the regression issues I was aware of. Please test it (especially the tags, because I could not yet replicate the scenario on my site). |
Details
Lots of simplifications:
useArrowKeyFocusManager
hook instead ofArrowKeyFocusManager
component, simplify lots of that codeuseActiveElementRole
anduseAutoFocusInput
useKeyboardShortcut
useMemo
. This also should come with some performance benefits to eliminate unnecessary extra renders and data mapping.usePrevious
utility instead of ad-hoc version of the same thing withuseRef
+useEffect
Fixed Issues
$ #37335
$ #37294
$ #37267
$ #37256
$ #37239
$ #37238
$ #37346
Tests / QA steps
Retest the following issues:
[HOLD for payment 2024-03-07] [HOLD][$500] [MEDIUM] Tags: All tags are revealed when searching for hidden tag and clearing the search field #37335
[$500] Expense - Unable to save Merchant by hitting Enter in confirmation page #37294
[$500] Chat - Incorrect search result & Show More' Dropdown shown when one search result displayed #37267
[HOLD for payment 2024-03-07] [$500] Group - After selecting user in Group chat, the modal is not sliding upwards #37256
[$500] Workspace switcher - Both "Expensify" and another workspace are highlighted in WS switcher #37239
[HOLD for payment 2024-03-07] [$500] IOU - "To" destination is highlighted in confirmation page #37238
[HOLD for payment 2024-03-07] [$500] Category - Selected category does not appear as selected in split request Category list #37346
Verify that no errors appear in the JS console
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 methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Verified #37256 is fixed:
web.mov
MacOS: Desktop