Skip to content

Comments

[material-ui][Chip] Add slots and slotProps#46098

Merged
sai6855 merged 13 commits intomui:masterfrom
sai6855:chip-slots
May 29, 2025
Merged

[material-ui][Chip] Add slots and slotProps#46098
sai6855 merged 13 commits intomui:masterfrom
sai6855:chip-slots

Conversation

@sai6855
Copy link
Member

@sai6855 sai6855 commented May 7, 2025

This PR adds slots and slotProps to Chip component

@sai6855 sai6855 changed the title Chip-slots [material-ui][Chip] Add slots and slotProps May 7, 2025
@sai6855 sai6855 added scope: chip Changes related to the chip. package: material-ui type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels May 7, 2025
@mui-bot
Copy link

mui-bot commented May 7, 2025

Netlify deploy preview

https://deploy-preview-46098--material-ui.netlify.app/

@mui/material/Chip: parsed: +2.16% , gzip: +2.06%
@mui/material/Autocomplete: parsed: +0.22% , gzip: +0.19%
@mui/material: parsed: +0.06% , gzip: +0.06%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against b26ec2f

@sai6855 sai6855 marked this pull request as draft May 7, 2025 07:42
@sai6855 sai6855 requested a review from siriwatknp May 7, 2025 11:47
@sai6855 sai6855 marked this pull request as ready for review May 7, 2025 11:47
},
ownerState,
ref: handleRef,
shouldForwardComponentProp: true,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
shouldForwardComponentProp: true,

ChipRoot comes from div, no need to forward component prop

Copy link
Member Author

@sai6855 sai6855 May 20, 2025

Choose a reason for hiding this comment

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

The component prop was forwarded because, when shouldForwardComponentProp is set to false, the component prop returned from useSlot('root') gets removed in the useSlot.ts file and replaced with the as prop here.

While this behavior works fine for components that do not rely on component-specific logic, it breaks components like Chip, which access the component prop directly in their logic — for example, this line. Removing the component prop in such cases causes test failures.

To fix this, I added shouldForwardComponentProp: true to ensure that the component prop is preserved and Chip continues to behave correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Can you add a comment above the line shouldForwardComponentProp.

Copy link
Member Author

Choose a reason for hiding this comment

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

done, added here 29b9ea4

@siriwatknp
Copy link
Member

@sai6855 Can you check the CIs? Some are failing.

@sai6855 sai6855 requested a review from siriwatknp May 20, 2025 08:39
label: {
expectedClassName: classes.label,
},
},
Copy link
Member

@siriwatknp siriwatknp May 21, 2025

Choose a reason for hiding this comment

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

Given the comment about the shouldForwardComponentProp. I think we need to add more tests.

  • With clickable: true and a custom component prop.

Copy link
Member Author

Choose a reason for hiding this comment

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

it('should render link with the button base', () => {
const { container } = render(<Chip component="a" clickable label="My text Chip" />);
expect(container.firstChild).to.have.class('MuiButtonBase-root');
expect(container.firstChild).to.have.tagName('a');
});
There is test which tests with same props combination as mentioned. is this test sufficient or do you have any other test in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Good one.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks for working on this.

Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
Signed-off-by: sai chand <60743144+sai6855@users.noreply.github.com>
@sai6855 sai6855 merged commit e92232b into mui:master May 29, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: chip Changes related to the chip. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants