Skip to content
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

[Menu] Improve performance and add support for variants #15360

Merged
merged 3 commits into from
Apr 23, 2019

Conversation

ryancogswell
Copy link
Contributor

@ryancogswell ryancogswell commented Apr 15, 2019

Closes #10847
Closes #14483
Closes #13708
Closes #13464

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 15, 2019

@material-ui/core: parsed: +0.20% , gzip: +0.34%

Details of bundle changes.

Comparing: 004bb03...4946af9

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.20% 🔺 +0.34% 🔺 310,305 310,924 83,864 84,145
@material-ui/core/Paper 0.00% -0.03% 67,276 67,277 19,971 19,965
@material-ui/core/Paper.esm 0.00% +0.03% 🔺 60,639 60,639 18,886 18,891
@material-ui/core/Popper 0.00% +0.01% 🔺 31,114 31,114 10,799 10,800
@material-ui/core/Textarea 0.00% -0.04% 5,472 5,472 2,370 2,369
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,898 15,898 5,771 5,771
@material-ui/core/useMediaQuery 0.00% +0.10% 🔺 2,106 2,106 973 974
@material-ui/lab 0.00% +0.01% 🔺 140,718 140,719 42,603 42,606
@material-ui/styles 0.00% +0.01% 🔺 50,831 50,831 15,018 15,019
@material-ui/system 0.00% -0.03% 11,765 11,765 3,925 3,924
Button 0.00% +0.01% 🔺 85,620 85,620 25,616 25,618
Modal 0.00% -0.01% 79,229 79,229 23,936 23,933
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,154 51,154 11,250 11,250
docs.main +0.03% 🔺 +0.10% 🔺 647,840 648,059 201,994 202,187
packages/material-ui/build/umd/material-ui.production.min.js +0.08% 🔺 +0.18% 🔺 291,845 292,088 81,971 82,119

Generated by 🚫 dangerJS against 4946af9

packages/material-ui/src/ListItem/ListItem.js Show resolved Hide resolved
packages/material-ui/src/ListItem/ListItem.js Outdated Show resolved Hide resolved
packages/material-ui/src/ListItem/ListItem.js Outdated Show resolved Hide resolved
packages/material-ui/src/Menu/Menu.js Outdated Show resolved Hide resolved
packages/material-ui/src/ListItem/ListItem.js Outdated Show resolved Hide resolved
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done a quick preliminary review.

I have noticed a focus ring. I think that we can hide it as we do with the Drawer or Dialog component. It's not ideal but given something pop up on the screen, the user has a lead on where the active element might be.

Capture d’écran 2019-04-16 à 17 29 49

packages/material-ui/src/ListItem/ListItem.js Outdated Show resolved Hide resolved
packages/material-ui/src/ListItem/ListItem.js Outdated Show resolved Hide resolved
packages/material-ui/src/ListItem/ListItem.js Outdated Show resolved Hide resolved
packages/material-ui/src/Menu/Menu.js Outdated Show resolved Hide resolved
packages/material-ui/src/ListItem/ListItem.js Show resolved Hide resolved
packages/material-ui/src/Menu/Menu.js Outdated Show resolved Hide resolved
@ryancogswell
Copy link
Contributor Author

I have noticed a focus ring.

@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 Menu modelling somewhat off of the approach for paper. Let me know if you want any details on this adjusted.

@ryancogswell
Copy link
Contributor Author

@oliviertassinari and @eps1lon I believe all of the review points have been addressed. Let me know if there are any remaining concerns.

@ryancogswell
Copy link
Contributor Author

@oliviertassinari I ended up needing to use a slightly different heuristic for skipping menu items than I initially planned. I mistakenly believed that tabIndex would be undefined in the DOM if it wasn't explicitly specified and wasn't an inherently tabbable element. This is not the case. At least in Chrome it defaults to -1. In IE an hr element (e.g. Divider) seems to default to 0.

So instead of skipping items with an undefined tabIndex, I'm skipping items where the tabIndex is less than -1. Then for disabled menu items and dividers, I'm setting the tabIndex to -2. This feels a little bit odd, but I don't have any ideas right now for anything more elegant and what I have does work.

@ryancogswell
Copy link
Contributor Author

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.

@oliviertassinari oliviertassinari force-pushed the menulist-tabindex branch 2 times, most recently from 29e5beb to 8467cea Compare April 21, 2019 21:43
@oliviertassinari
Copy link
Member

@ryancogswell I'm rebasing on next.

@oliviertassinari oliviertassinari force-pushed the menulist-tabindex branch 6 times, most recently from 2819c5c to 6766154 Compare April 21, 2019 23:14
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really well done 💪

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module! PR: good for merge labels Apr 21, 2019
@ryancogswell
Copy link
Contributor Author

Really well done 💪

Thanks!

@oliviertassinari oliviertassinari merged commit 8ee6af6 into mui:next Apr 23, 2019
@ryancogswell ryancogswell deleted the menulist-tabindex branch May 3, 2019 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module!
Projects
None yet
4 participants