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] Migrate MenuItem to emotion #24631

Merged
merged 15 commits into from Feb 18, 2021
Merged

Conversation

xs9627
Copy link
Contributor

@xs9627 xs9627 commented Jan 26, 2021

This PR migrates the MenuItem component to the new emotion format as a part of #24405.

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 26, 2021

@material-ui/core: parsed: +0.16% , gzip: +0.12%

Details of bundle changes

Generated by 🚫 dangerJS against 354ff22

@oliviertassinari oliviertassinari added the component: menu This is the name of the generic UI component, not the React module! label Jan 26, 2021
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I believe we should implement this by adding components and componentsProps in the ListItem component, similar to how we done it in #24634 cc @oliviertassinari

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 27, 2021

@mnajdova On a side note, we think that we should remove MenuList > ListItem in v5 because the abstraction makes too little sense.

@mnajdova
Copy link
Member

@mnajdova On a side note, we think that we should remove MenuList > ListItem in v5 because the abstraction makes too little sense.

In that case should we hold off with these changes until the component is refactored?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 27, 2021

In that case should we hold off with these changes until the component is refactored?

@mnajdova I have repurposed #21587 to keep track of this effort. I don't know when it will happen. I think that it would be more valuable to first remove JSS from the list of dependencies entirely. Having to implement the components/componentsProps makes us a step further in the unstyled direction we are pursuing

@mnajdova
Copy link
Member

@mnajdova I have repurposed #21587 to keep track of this effort. I don't know when it will happen. I think that it would be more valuable to first remove JSS from the list of dependencies entirely. Having to implement the components/componentsProps makes us a step further in the unstyled direction we are pursuing

Makes sense! @xs9627 would you like to proceed with this? #24634 should be a good template for introducing components and componentsProps and using them in a component.

@xs9627
Copy link
Contributor Author

xs9627 commented Jan 27, 2021

Not quite sure about the changes, just saw all tests passed for MenuItem and ListItem :)

@xs9627 xs9627 requested a review from mnajdova January 31, 2021 10:37
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 11, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 18, 2021
@oliviertassinari oliviertassinari merged commit 22491f2 into mui:next Feb 18, 2021
@oliviertassinari
Copy link
Member

@xs9627 Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants