Skip to content

Conversation

@mnajdova
Copy link
Member

@mnajdova mnajdova commented Sep 3, 2024

Fixes #43588

@mnajdova mnajdova added scope: radio Changes related to the radio. type: regression A bug, but worse, it used to behave as expected. 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?

@oliviertassinari oliviertassinari added the type: bug It doesn't behave as expected. 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 a team and oliviertassinari 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 😊

@oliviertassinari oliviertassinari removed the type: bug It doesn't behave as expected. label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: radio Changes related to the radio. type: regression A bug, but worse, it used to behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Selected radio buttons are still colored even when disabled

4 participants