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

Fix selected checkbox fill color when checkbox group is disabled #5407

Merged
merged 3 commits into from Nov 13, 2023

Conversation

yihuiliao
Copy link
Collaborator

@yihuiliao yihuiliao commented Nov 10, 2023

Closes #5378

After investigating this a bit more, I don't think this is a css specificity issue. It seemed that when the isDisabled prop was provided to<CheckboxGroup>, that value was not passed to the individual <Checkbox> components, resulting in the absence of the is-disabled class in their classnames. Therefore, if you wanted to the disabled styles to be applied to a selected checkbox in a checkbox group, you would also have to directly pass isDisabled to that specific checkbox.

Looking back at the commit history, it appears that this "broke" with this PR (#3827). However, if you go commit before it (which is #3962), you'll notice that the styling might be correct, but the is-disabled class still isn't a part of the classname even though it should be.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. Go to the CheckboxGroup in storybook
  2. Use the controls and play around the different states when isDisabled is true
  3. It should have the correct fill color when it is disabled

🧢 Your Project:

@rspbot
Copy link

rspbot commented Nov 10, 2023

@yihuiliao yihuiliao marked this pull request as ready for review November 11, 2023 00:01
Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

Tested on chrome and iphone and wasn't able to find a case where it wasn't correct.

ktabors
ktabors previously approved these changes Nov 11, 2023
@rspbot
Copy link

rspbot commented Nov 13, 2023

@rspbot
Copy link

rspbot commented Nov 13, 2023

@rspbot
Copy link

rspbot commented Nov 13, 2023

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@yihuiliao yihuiliao merged commit 3efd6c0 into main Nov 13, 2023
26 checks passed
@yihuiliao yihuiliao deleted the checkbox-disabled branch November 13, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selected checkbox in a disabled checkbox group has the wrong fill color
5 participants