Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
use PopoverMenu for account switcher modal #49051
Changes from 47 commits
e46b607
7069e8c
eb9e18b
e894d78
de3bf0f
1a5aaa7
a49cada
5be7a19
95f4224
dd32f45
70aa7f3
43e1ff1
bed23eb
4002bc1
2d5c678
584469a
a9da05f
9b9e00f
7bc72ef
3ac507c
5d6c9db
366bce2
e2ee3c7
6556abd
907b6e0
84291a0
3a09746
6820937
a58ff31
4fde281
8692d40
1e8b7ce
b326494
c3ed169
92d3081
a6770f8
36b7519
25547dc
ab098d6
69fefcd
08bde40
be68a72
c61aa1a
4e20be9
492ea92
a2f61c6
e538a07
f7ffa39
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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.
innerContainerStyle
ofBaseModal
has style isViewStyle
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.
another point, other style prop names don't have
s
at the end, should we renameheaderStyles
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.
Hey 👋 , coming from this issue. We changed the
menuItems.length
comparison to a deep comparison ofmenuItems
, which may be causing the Popover menu flicker by re-rendering the component. We fixed it by using the existingshouldFreezeCapture
prop in theActiveHoverable
component. However, if we use only the length comparison instead of a deep comparison, the issue doesn’t seem to occurThere 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.
Using the length is not enough to check if the items changed. I'm not sure if the freeze approach is best here. I think it's better to investigate why it did re-render, if the list was not changed you should memoise it.
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.
need to remove this before merging