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.

Sign up for GitHub

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

Jump to bottom

[MenuItem]: add prop for ListItem classes #15161

Closed
wants to merge 1 commit into from
Closed

[MenuItem]: add prop for ListItem classes #15161

wants to merge 1 commit into from

Conversation

zsalzbank
Copy link
Contributor

As far as I could tell, there was no way to modify the ListItem classes. I used the same methodology as the Menu does for changing the child Popover classes.

zsalzbank
zsalzbank commented Apr 2, 2019
View reviewed changes
packages/material-ui/src/MenuItem/MenuItem.js
@@ -28,7 +28,16 @@ export const styles = theme => ({
});

const MenuItem = React.forwardRef(function MenuItem(props, ref) {
const { classes, className, component, disableGutters, role, selected, ...other } = props;
const {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This got reformatted when I did yarn prettier.

@oliviertassinari
Copy link
Member

@zsalzbank What's your objective?

@zsalzbank
Copy link
Contributor Author

I'd like to be able to change the focusVisible class so that it matches the menu item hover when navigating with the keyboard. Figured other components expose similar functionality, so this couldn't hurt.

@mui-pr-bot
Copy link

Details of bundle changes.

Comparing: 240dc18...ee226f0

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.01% 🔺 +0.02% 🔺 349,839 349,887 89,879 89,897
@material-ui/core/Paper 0.00% 0.00% 67,853 67,853 19,820 19,820
@material-ui/core/Paper.esm 0.00% 0.00% 60,184 60,184 18,563 18,563
@material-ui/core/Popper 0.00% 0.00% 30,463 30,463 10,530 10,530
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,364 17,364 5,726 5,726
@material-ui/core/useMediaQuery 0.00% 0.00% 2,469 2,469 1,041 1,041
@material-ui/lab 0.00% 0.00% 147,758 147,758 43,621 43,621
@material-ui/styles 0.00% 0.00% 53,105 53,105 15,433 15,433
@material-ui/system 0.00% 0.00% 17,136 17,136 4,525 4,525
Button 0.00% 0.00% 87,927 87,927 26,054 26,054
Modal 0.00% 0.00% 82,035 82,035 24,552 24,552
colorManipulator 0.00% 0.00% 3,232 3,232 1,299 1,299
docs.landing 0.00% 0.00% 49,899 49,899 10,803 10,803
docs.main +0.01% 🔺 +0.01% 🔺 645,314 645,362 200,459 200,477
packages/material-ui/build/umd/material-ui.production.min.js +0.02% 🔺 +0.02% 🔺 298,242 298,290 82,697 82,711

Generated by 🚫 dangerJS against ee226f0

@oliviertassinari
Copy link
Member

Why not using the focusVisibleClassName prop?

@oliviertassinari
Copy link
Member

If we move forward with #15140. You will no longer need it. At least, in the most simple case, you could target .focus-visible.

@zsalzbank
Copy link
Contributor Author

Why not using the focusVisibleClassName prop?

That works - I guess I didn't notice it because it was so far down the chain. It would be cool if the documentation was expandable or something so that all props (nested and far down the chain) were exposed on one page.

@zsalzbank
Copy link
Contributor Author

Feel free to close if this isn't desirable.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 2, 2019 •
edited
Loading

I think that we should limit as much as possible the nested classes API usage. The focusVisibleClassName property was added for your use case. We can limit the API surface this way.

It would be cool if the documentation was expandable or something so that all props (nested and far down the chain) were exposed on one page.

We agree, we were thinking of documenting the whole prop spread chain. In the case of the MenuItem, it would be: MenuItem > ListItem > ButtonBase > button.

@oliviertassinari
Copy link
Member

@zsalzbank I'm closing for now. Thank you for taking the time.

@zannager zannager added the component: menu This is the name of the generic UI component, not the React module! label Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component: menu This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zsalzbank @oliviertassinari @mui-pr-bot @zannager