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-ui][FormControl] Fix issue with the conditional setting of htmlFor and id attributes not functioning properly for form labels #40180

Conversation

ReaZzy
Copy link
Contributor

@ReaZzy ReaZzy commented Dec 11, 2023

As mentioned by PhillipWinder in the bug report, if either the 'id' or 'htmlFor' is undefined, they are currently removed from the DOM instead of being set to their default values. This issue arises due to externalForwardedProps overriding additionalProps. To address this, we can fix the problem by conditionally adding htmlFor / id to externalForwardedProps. Additionally, I have included tests to verify that the behaviour with undefined values is now correct.

Fixes #40112.

@mui-bot
Copy link

mui-bot commented Dec 11, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against f3cec0b

@danilo-leal danilo-leal added package: joy-ui Specific to @mui/joy component: FormControl The React component labels Dec 12, 2023
@DarhkVoyd
Copy link
Contributor

Hey @ReaZzy,

I have a few suggestions, your PR currently lacks a description. To help the reviewers understand the changes you've made, could you please add a brief description that outlines the purpose of the PR, any proposed changes, and if applicable, mention if you've added or modified tests.

Additionally, as this PR addresses a specific issue, you can link it by adding Closes: #40112 in your description to link this PR to the original issue. This helps in tracking and closing the associated issue once the PR is merged.

Thanks for your contribution! A member of the team will review your PR soon.

@ZeeshanTamboli ZeeshanTamboli added the bug 🐛 Something doesn't work label Dec 15, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [joy-ui][FormLabel] Add undefined checks for htmlFor and id attributes [joy-ui][FormLabel] Fix htmlFor and id attributes not working for form label Dec 15, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [joy-ui][FormLabel] Fix htmlFor and id attributes not working for form label [joy-ui][FormControl] Fix htmlFor and id attributes not working for form label Dec 15, 2023
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.

@ReaZzy I moved the tests out of the Input test block since the Input component isn't rendered. Everything seems fine to me now! It's a great first pull request at MUI!

@ZeeshanTamboli ZeeshanTamboli changed the title [joy-ui][FormControl] Fix htmlFor and id attributes not working for form label [joy-ui][FormControl] Fix issue with the conditional setting of htmlFor and id attributes not functioning properly for form labels Dec 15, 2023
@ZeeshanTamboli ZeeshanTamboli merged commit 73d2a24 into mui:master Dec 15, 2023
19 checks passed
@ReaZzy ReaZzy deleted the 40112-htmlfor-and-id-attribute-disappears-when-set-to-undefined branch December 15, 2023 14:51
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: FormControl The React component package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[joy-ui][FormLabel] htmlFor attribute not working as expected (disappears when set to undefined)
5 participants