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

[Chip][material-next] Add Material You Chip component #38927

Merged
merged 18 commits into from
Sep 26, 2023

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Sep 11, 2023

Chip issue: #38024
Material You umbrella issue: #29345

Closes: #38024

Changes

  • Migrate to Typescript
  • Drop ThemeProvider in favor of CssVarsProvider
  • Implement Material You design specs
  • Add component playground to v5 docs
  • Refactor styles to use component CSS Variables
  • Drop composed classes

Breaking changes

https://github.com/mui/material-ui/pull/38927/files#diff-9548901bac6f3274c402a36fad9b83afe49cf663499e51d9fe306195d149a4a1

Playground

Link: https://deploy-preview-38927--material-ui.netlify.app/material-ui/react-chip/#material-you-version

@DiegoAndai DiegoAndai added component: chip This is the name of the generic UI component, not the React module! v6.x design: material you labels Sep 11, 2023
@mui-bot
Copy link

mui-bot commented Sep 11, 2023

Netlify deploy preview

@mui/material-next: parsed: -0.33% 😍, gzip: +0.86%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 9e1f375

@DiegoAndai DiegoAndai marked this pull request as ready for review September 12, 2023 20:00
@DiegoAndai DiegoAndai self-assigned this Sep 12, 2023
Copy link
Member

@mj12albert mj12albert left a comment

Choose a reason for hiding this comment

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

Just one minor comment – I noticed that the MD3 "Filter chip" (spec) allows a trailing icon, which is not in the MD2 spec

Otherwise, looks great! 👍

Copy link
Contributor

@DavidCnoops DavidCnoops left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me. Great job, and a very nice next step in bringing material v3 to Material UI!

Couple of questions/remarks:

  • The reference spec includes custom leading and trailing icons. It's unclear to me if this is currently supported by the component. If so, it might make sense to include an example in the playground.
  • The material v3 design kit includes examples of Chips with fully rounded corners. Is this something we want to support out-of-the-box as well?
  • The focus state on the deletable chip seems to be targeting the entire chip instead of just the close button. It also appears the close/delete button does not get triggered by an enter keypress.
  • In light mode the primary and secondary color chips look almost indistinguishable from each other (only the text color appears to change very slightly). In dark mode the difference is much more noticeable.
  • The default color (when no color prop is defined) appears to be secondary. Should this be primary instead?

@mj12albert
Copy link
Member

  • In light mode the primary and secondary color chips look almost indistinguishable from each other

I also noticed this in my review – I checked the color values and they are correct, the difference is just really hard to see at the size of the Chip 😅

Screenshot 2023-09-20 at 8 22 47 PM

@DiegoAndai
Copy link
Member Author

Thanks for the review @mj12albert and @DavidCnoops. Regarding your feedback:

  • Rounded and circular shape: There's already a PR for implementing something similar (the other way around) in v5: [Chip] adding 'rounded' property to chip component #37576. Let's implement that change in v5 and port it to v6. Do you agree? I'll follow up on that PR so it gets merged soon.

  • Colors: The secondary color is used in the specs, as the chips are intended to have less prominence than other elements, buttons, for example. That's why it should be the default. Regarding the difference between primary and secondary, I think that's intended. The secondary in MD3 is intended for "less prominent components". They used a similar hue, as using a different one might make the elements more prominent, which is the case in MD2.

  • Leading and trailing icons: The specs indeed support trailing icons. I think it's worth it to refactor the Chip component to implement the slots pattern, deprecating the current avatar, icon, and deleteIcon props. After that refactor, we can develop a better API to support leading/trailing decorations. We should create a separate issue to keep this PR manageable.

  • Focus state: This is a difficult one, and the accessibility specs don't help much. For deletable Chips, should the focus be:

    1. Land on the entire deletable chip and be deleted with backspace/delete (this is how v5 works)
    2. Land only on the delete icon and be deleted with space/enter
    3. Land only on the delete icon and be deleted with space/enter/backspace/delete
    4. Land on both, deleting with backspace/delete if the focus is on the entire chip and with space/enter if the focus is on the delete icon

    This also relates to the leading/trailing icons point above: Right now, we support delete icons on the trailing spot, but there might be other action icons used there, in which case the backspace/delete shouldn't activate the action. Overall, we need a separate issue to design a proper API. We could do this alongside the slots support.

Let me know what you think.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 22, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 22, 2023
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.

Small things I noticed, the component iself looks good to me 👍

@DiegoAndai DiegoAndai merged commit fad8a26 into mui:master Sep 26, 2023
@DiegoAndai DiegoAndai deleted the material-you-chip branch September 26, 2023 13:54
@DiegoAndai DiegoAndai added v7.x and removed v6.x labels Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: chip This is the name of the generic UI component, not the React module! design: material you v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Chip][material-next] Add Chip component
5 participants