-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[Menu] Improve performance and add support for variants #15360
Conversation
@material-ui/core: parsed: +0.20% , gzip: +0.34% Details of bundle changes.Comparing: 004bb03...4946af9
|
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.
@oliviertassinari I have addressed this, but MenuList doesn't have any of its own classes, so I was a bit uncertain on where to control this and how to name/document the css class. I put it in |
@oliviertassinari and @eps1lon I believe all of the review points have been addressed. Let me know if there are any remaining concerns. |
@oliviertassinari I ended up needing to use a slightly different heuristic for skipping menu items than I initially planned. I mistakenly believed that So instead of skipping items with an undefined |
9f1cbf4
to
11b6652
Compare
I believe my current MenuList implementation has the potential to cause an infinite loop if all of the menu items are disabled. I'll add a test and fix tonight. |
29e5beb
to
8467cea
Compare
@ryancogswell I'm rebasing on next. |
2819c5c
to
6766154
Compare
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.
Really well done 💪
Thanks! |
e6ab67e
to
4946af9
Compare
Closes #10847
Closes #14483
Closes #13708
Closes #13464