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

[Radio] Fix disabled state styling regression #43592

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Sep 3, 2024

Fixes #43588

@mnajdova mnajdova added component: radio This is the name of the generic UI component, not the React module! regression A bug, but worse labels Sep 3, 2024
@mui-bot
Copy link

mui-bot commented Sep 3, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 34a9162

@mnajdova mnajdova marked this pull request as ready for review September 3, 2024 22:57
@mnajdova mnajdova requested a review from a team September 3, 2024 22:58
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Tricky, if we want the customization experience to be consistent with the other components, e.g. Button, it seems that adding the disabled style in

[`&.${radioClasses.checked}`]: {
would make more sense. But the real question is what's better DX between the two options?

packages/mui-material/src/Radio/Radio.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 3, 2024
@mnajdova
Copy link
Member Author

mnajdova commented Sep 4, 2024

Yeah, my first attempt was to not to do this, with 064907e, but the problem was that we will need to use the useFormControl hook here as well, while it is still used in SwitchBase to ensure we have other right disabled value.

In the end that may be the best out of the two possible solutions. I will update the PR today.

@mnajdova
Copy link
Member Author

mnajdova commented Sep 4, 2024

I've updated the PR to not change the specificity.

@mnajdova mnajdova requested review from oliviertassinari and a team and removed request for oliviertassinari September 4, 2024 09:50
let disabled = disabledProp;

if (muiFormControl) {
if (typeof disabled === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

Why only read the value if undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the prop should take precedence over what comes form the form field context.

Copy link
Member

Choose a reason for hiding this comment

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

Right, my bad 😅 I got confused and thought we were checking typeof muiFormControl.disabled === 'undefined'

@DiegoAndai
Copy link
Member

@mnajdova
Copy link
Member Author

mnajdova commented Sep 6, 2024

@mnajdova, do you know how this broke?

@DiegoAndai can you clarify what you mean? The before primary style on the radio item is not expected, this is what the PR fixes. Unless there is something else. See v5: https://codesandbox.io/p/sandbox/staging-violet-dh3r6m?file=%2Fpackage.json%3A21%2C33&workspaceId=836800c1-9711-491c-a89b-3ac24cbd8cd8

@DiegoAndai
Copy link
Member

@mnajdova I mean: How was this bug introduced? Did we miss something when migrating to the variants API?

I'm trying to understand what was the change that introduced the regression to see if there might be similar issues elsewhere.

@mnajdova
Copy link
Member Author

mnajdova commented Sep 7, 2024

I'm trying to understand what was the change that introduced the regression to see if there might be similar issues elsewhere.

Typically previously we were adding the disabled style last, however, now that we have variants, the variants are being processed after the "default styles" always. In practise this means, that previously we had:

const A = styled()({
  // this is going to be added first
  '&.checked': { 
    // checked styles
  },
  // this is going to be added second, so it always wins
  '& .disabled': { 
    //disabled styles
  },
})

And now with the changes:

const A = styled('div')({
   // this is going to be added first as part of the default styles
  '& .disabled': { 
    //disabled styles
  },
  variants: [{
    props: { color: 'primary' },
    style: { 
      // this is going to be second and because it has the same
      // specificity, wins over the disabled styles
      '&.checked': { 
        // checked styles
      },
    }
  }]
})

Does this helps explain the issue?

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Does this helps explain the issue?

It does! thanks 😊

@DiegoAndai DiegoAndai merged commit 98be46d into mui:master Sep 9, 2024
19 checks passed
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: radio This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selected radio buttons are still colored even when disabled
4 participants