-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
use PopoverMenu for account switcher modal #49051
base: main
Are you sure you want to change the base?
Conversation
@s77rt Fixed two bugs above. About the first bug, the reason is the wrap view of |
@s77rt Can't reproduce the first bug. Screen.Recording.2024-09-16.at.18.04.40.mov |
@nkdengineer Close the modal and open it again |
69f4a85
to
70aa7f3
Compare
@s77rt I updated. Please help to check again. |
@nkdengineer Please fix lint issues |
Fixed |
@nkdengineer Can you look into the performance failing test |
@s77rt The reason is we change this line |
@nkdengineer I don't think this is acceptable as I doubt the list changed during the test. It's most likely due to shallow comparison where |
Let me try. |
@s77rt Still fails with |
@nkdengineer Please investigate why it fails i.e. what were the previous and the next |
@s77rt It comes from |
src/components/PopoverMenu.tsx
Outdated
@@ -336,7 +336,7 @@ PopoverMenu.displayName = 'PopoverMenu'; | |||
export default React.memo( | |||
PopoverMenu, | |||
(prevProps, nextProps) => | |||
lodashIsEqual(prevProps.menuItems, nextProps.menuItems) && | |||
lodashIsEqual(prevProps.menuItems.length, nextProps.menuItems.length) && |
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.
Don't we need to compare the menu items and not just their lengths in order to show the error
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.
@s77rt Sorry, that was my mistake. I forgot to revert the change in PopoverMenu
when running perf-test locally and mistakenly thought it was fixed. But the current fail test come from AttachmentPickerWithMenuItems
how are we looking on this one? |
@s77rt Since the render count difference is only |
@nkdengineer I think we should at least find the root cause i.e. what's causing the extra render |
@nkdengineer Any progress on this? |
Still investigating the RCA. |
Details
use PopoverMenu for account switcher modal
Fixed Issues
$ #48292
PROPOSAL: #48292 (comment)
Tests
Offline tests
Same as above
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
Android: Native
Screen.Recording.2024-09-12.at.16.37.31.mov
Android: mWeb Chrome
Screen.Recording.2024-09-12.at.16.41.41.mov
iOS: Native
Screen.Recording.2024-09-12.at.16.42.31.mov
iOS: mWeb Safari
Screen.Recording.2024-09-12.at.16.38.51.mov
MacOS: Chrome / Safari
Screen.Recording.2024-09-12.at.16.36.32.mov
MacOS: Desktop
Screen.Recording.2024-09-12.at.16.46.39.mov