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

[material-ui][Rating] Use Rating name as prefix of input element ids #43829

Merged

Conversation

yash49
Copy link
Contributor

@yash49 yash49 commented Sep 20, 2024

fixes #40997

@mui-bot
Copy link

mui-bot commented Sep 20, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against c2eb034

@yash49 yash49 changed the title [mui-material][Rating] Use Rating name as prefix of generated id-s [mui-material][Rating] Use Rating name as prefix of input element ids Sep 20, 2024
@ZeeshanTamboli ZeeshanTamboli added package: material-ui Specific to @mui/material component: rating This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Sep 27, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [mui-material][Rating] Use Rating name as prefix of input element ids [material-ui][Rating] Use Rating name as prefix of input element ids Sep 27, 2024
@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review September 27, 2024 10:54
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Looks good. We also need to add unit test for it.

packages/mui-material/src/Rating/Rating.js Outdated Show resolved Hide resolved
@yash49
Copy link
Contributor Author

yash49 commented Sep 27, 2024

Looks good. We also need to add unit test for it.

Sure, will add one & update the PR

@yash49
Copy link
Contributor Author

yash49 commented Sep 27, 2024

Looks good. We also need to add unit test for it.

@ZeeshanTamboli I've added the unit test. Let me know if anything needs an update.
Also argos has detected changes but they are not related to my PR.

// preventing one component from affecting another. React 18's useId already handles this.
// Update to const id = useId(); when React 17 support is dropped.
// More details: https://github.com/mui/material-ui/issues/40997
const id = `${name}-${useId()}`;
Copy link
Member

Choose a reason for hiding this comment

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

I changed it to this since name will always be available either through provided prop or generated through useId below.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @yash49. I also fixed a typo in the API description.

@ZeeshanTamboli ZeeshanTamboli merged commit a029ed7 into mui:master Sep 28, 2024
22 checks passed
@yash49
Copy link
Contributor Author

yash49 commented Sep 28, 2024

Thanks @ZeeshanTamboli 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: rating This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][Rating] Component does not respect name property when generating input element id
3 participants