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

[Joy] Add Chip component #31983

Merged
merged 37 commits into from
Apr 20, 2022
Merged

[Joy] Add Chip component #31983

merged 37 commits into from
Apr 20, 2022

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Mar 25, 2022

This PR includes:

  • correcting typos in Joy SvgIcon demo / Joy Avatar demo / Joy Avatar API code
  • addition of Joy Chip component
  • tests for Joy Chip component

Props: startDecorator, endDecorator, color, size, variant, disabled

Default values:

  • color: primary
  • size: md
  • variant: contained
  • disabled: false

I removed onDelete, icon, avatar, label, clickable and deleteIcon props and added startDecorator / endDecorator following up with this discussion

Preview: https://deploy-preview-31983--material-ui.netlify.app/experiments/joy/chip/

Lines of code comparison

Material Joy % change
Chip 560 350 -37.50
ChipDelete 148
total 560 498 -11.07

@mui-bot
Copy link

mui-bot commented Mar 25, 2022

Details of bundle changes

@mui/joy: parsed: +5.93% , gzip: +3.81%

Generated by 🚫 dangerJS against e151fb6

@hbjORbj hbjORbj added component: chip This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Mar 25, 2022
@siriwatknp
Copy link
Member

siriwatknp commented Mar 25, 2022

From the issues that I can find:

I am thinking about these changes

  1. Can we get rid of the label prop and use the same old children prop? This will give the same experience across other components. However, the slot label is needed to create text-overflow ellipsis.
// implementation
<ChipRoot>
 ...startDecorator
  <ChipLabel>{children}</ChipLabel>
 ...endDecorator
</ChipRoot>

// usage
<Chip>text</Chip>
  1. introduce ChipButton, and ChipDelete component instead of using clickable, onDelete and deleteIcon prop. This would make the Chip API cleaner since it already exposes start/endDecorator. Here is what the usage looks like in [RFC] Chip markup #20470 (comment)

cc @hbjORbj @danilo-leal @mnajdova

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

introduce ChipButton, and ChipDelete component instead of using clickable, onDelete and deleteIcon prop.

This is a great idea. Other than that, it's looking pretty good to me!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 27, 2022
@siriwatknp
Copy link
Member

@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 29, 2022

@siriwatknp

Can we get rid of the label prop and use the same old children prop? This will give the same experience across other components. However, the slot label is needed to create text-overflow ellipsis.

Yes, I can do that.

introduce ChipButton, and ChipDelete component instead of using clickable, onDelete and deleteIcon prop. This would make the Chip API cleaner since it already exposes start/endDecorator. Here is what the usage looks like in #20470 (comment)

Yes, I agree it looks better. I will work on it.

Could you replicate some of these design inspirations in the /experiments/joy/chip/ page to verify the flexibility of the new Chip component?

Sure, no problem.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 31, 2022
@hbjORbj hbjORbj force-pushed the joy/chip branch 2 times, most recently from 55bbcdc to 134fa24 Compare April 4, 2022 11:28
@hbjORbj
Copy link
Member Author

hbjORbj commented Apr 14, 2022

@siriwatknp

Here are updates:

  • Changed default value for variant and color to contained and primary, respectively
  • Created a ChipDelete component
  • Replaced ButtonBase with a HTML button when clickable is true
  • Made the style more solid
  • Added more examples

@hbjORbj hbjORbj requested a review from danilo-leal April 14, 2022 22:37
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Great start! I'll probably tweak some styles later on but this is excellent already :)

@siriwatknp
Copy link
Member

Summary of my changes

  • Component variables:
    • remove some variables that are not necessary, eg --Chip-color, --Chip-fontSize
    • add --Chip-delete-size, --Chip-radius to make customization easier
  • Markup:
    • add action slot for creating clickable chip
    • custom chip delete icon by providing children
  • Integration:
    • allow chip to automatically adjust avatar's radius
    • chip delete's radius also adjust to the chip's radius
    • the chip control the icon's font-size based on its size
    • chip delete's variant, color adapt to the root chip
  • Accesibility: follow [RFC] Chip markup #20470

@hbjORbj hbjORbj merged commit d14c4d9 into mui:master Apr 20, 2022
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! package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants