USWDS - Checkbox: Add indeterminate styles #5713
Conversation
Support [indeterminate checkboxes] through both the CSS `:indeterminate` selector and also the `data-indeterminate` attribute. Fixes #918. [indeterminate checkboxes]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox#indeterminate_state_checkboxes
…io-colors.scss Co-authored-by: mahoneycm <charlie.mahoney@bixal.com>
…eterminate-high-contrast
USWDS - Checkbox: Add styling for indeterminate checkboxes
…eterminate-high-contrast
…rminate-feature-branch
…wds/uswds into cm-cleanup-scss-and-stories
…eterminate-high-contrast
…wds/uswds into cm-checkbox-indeterminate-high-contrast
USWDS - Indeterminate Checkbox: Cleanup scss and stories
…wds/uswds into cm-checkbox-indeterminate-high-contrast
…ntrast USWDS - Checkbox: Create high contrast styles for indeterminate checkboxes
| &:disabled, | ||
| &[aria-disabled="true"] { | ||
| @include format-input { | ||
| box-shadow: $tile-box-shadow--disabled; | ||
| } | ||
|
|
||
| @include format-label { | ||
| border-color: $tile-border-color--disabled; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Disabled conflict fix
Moving the checkbox:indeterminate styles caused the newer format-input styles to take priority. These styles are required to maintain disabled box shadow and tile colors.
Alternatively, we can move these styles to the disabled styles defined in lines 182-191. I opted to move them here since they weren't required to be redefined for the disabled checkboxes without indeterminate.
| &:checked[aria-disabled="true"], | ||
| &:indeterminate:disabled, | ||
| &:indeterminate[aria-disabled="true"], | ||
| &[data-indeterminate]:disabled, | ||
| &[data-indeterminate][aria-disabled="true"] { | ||
| @include format-input { |
There was a problem hiding this comment.
Indeterminate checkbox styles alternate location
As mentioned above, we could add the styles from lines 150-160 to this block but the redefinition for checked:disabled, &:checked[aria-disabled="true"] isn't necessary.
Let me know if we'd prefer to move them here!
There was a problem hiding this comment.
Makes sense to leave as-is if checked:disabled, &:checked[aria-disabled="true"] don't apply.
| @@ -34,12 +49,19 @@ | |||
| $input-border-alpha: -0; | |||
| $tile-border-color: color("gray-20"); | |||
| $tile-border-color--disabled: color("gray-10"); | |||
| $tile-box-shadow--disabled: 0 0 0 units($theme-input-select-border-width) | |||
There was a problem hiding this comment.
New variable
Created this new variable so we can update the disabled tile box shadow in one location. Affects checked + indeterminate checkboxes, ignores indeterminate radio
| indeterminate: { | ||
| name: "Indeterminate option (toggle to reset)", | ||
| control: { type: "boolean" }, | ||
| defaultValue: false | ||
| } |
There was a problem hiding this comment.
New storybook control
Added new storybook control to radio so it's easy to test indeterminate or "unselected` radio styles.
Toggle the control to remove selection!
|
|
||
| export const Indeterminate = Template.bind({}); | ||
| Indeterminate.argTypes = { | ||
| indeterminate: { | ||
| defaultValue: true, | ||
| }, | ||
| }; | ||
|
|
||
| export const IndeterminateTile = TileTemplate.bind({}); | ||
| IndeterminateTile.argTypes = { | ||
| indeterminate: { | ||
| defaultValue: true, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Indeterminate radio stories
Added indeterminate radio stories since it was easy to do so using new control.
I chose to not deactivate the control so that it's easy to reset after a selection is made!
There was a problem hiding this comment.
Looks good to me! I added a couple of comments for optional changes to add clarity, but neither needs to be a blocker.
I performed the following tests:
- Check display of radio and checkbox on Chrome, Firefox, and Safari
- Confirm checkbox indeterminate stories are present and meet standards
- In standard mode, confirm styles are consistent with other checkbox states and meet standards
- In forced colors mode, confirm styles are consistent with other checkbox states and meet standards
- In disabled state, confirm styles are consistent with other checkbox states and meet standards
- Confirm no regressions in radio button component
- Confirm radio indeterminate stories are present and meet standards
- When
$theme-body-background-coloris set to a dark color (I tested “ink”), confirm styles are consistent with other checkbox states and meet standards - Confirm running
npm run prettier:js,npm run prettier:sassandnpm run lintdo not generate new fixes- Note: Found a small change and pushed it up
- Confirm that styling
data-indeterminateandindeterminateattributes make sense - Confirm code meets standards
| @@ -9,6 +9,11 @@ export default { | |||
| control: { type: "radio" }, | |||
| options: ["none", "disabled", "aria-disabled"], | |||
| }, | |||
| indeterminate: { | |||
| name: "Indeterminate option (toggle to reset)", | |||
There was a problem hiding this comment.
Optional
Not a blocker, but I am not sure I understand what "toggle to reset" means. Wondering if it is safe to remove?
| name: "Indeterminate option (toggle to reset)", | |
| name: "Indeterminate option", |
There was a problem hiding this comment.
The reason I added it is because you can still select radio buttons while the indeterminate is turned on. Toggling the switch will then properly reset the indeterminate state.
Here's a quick demo:
Kapture.2024-03-04.at.12.15.04.webm
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Unchecked radio buttons receive :indeterminate pseudo-class. | ||
| // Because of this, we need to explicetly target checkbox :indeterminate styles. |
There was a problem hiding this comment.
Optional
I'm not sure I understand this comment. Why are radio buttons referenced if this is only for checkbox styles?
There was a problem hiding this comment.
I thought this note would add clarity as to why we aren't including the styles below with lines 127-132 (which is where they were previously defined).
I wanted to document the decision here so that if future devs know to not move them in case they miss the radio:indeterminate regressions as we did in early testing.
If we don't think it's necessary, we can remove!
There was a problem hiding this comment.
It'd be helpful to speak to that in the comment.
// Adding indeterminate styles here avoids style conflicts from setting in lines 127-129.I wanted to document the decision here so that if future devs know to not move them in case they miss the radio:indeterminate regressions as we did in early testing.
Does that mean moving the styles to 127-132 will cause similar regressions with checkbox? If so, we can just change radio → checkbox because it looks like one affects the other.
There was a problem hiding this comment.
No issues with functionality, feature looks good.
Made small edits to the PR description and suggestions to improve code clarity.
@amyleadem @mahoneycm can you resolve/hide comments that no longer apply?
For example, was this resolved?
Is this captured in the PR description? Should we include it?
| @@ -9,6 +9,11 @@ export default { | |||
| control: { type: "radio" }, | |||
| options: ["none", "disabled", "aria-disabled"], | |||
| }, | |||
| indeterminate: { | |||
| name: "Indeterminate option (toggle to reset)", | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -16,6 +16,21 @@ | |||
| @include -checkbox-and-radio-colors($input-type: "radio", $args...); | |||
| } | |||
|
|
|||
| @mixin selected-tile-colors($input-active-color) { | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Unchecked radio buttons receive :indeterminate pseudo-class. | ||
| // Because of this, we need to explicetly target checkbox :indeterminate styles. |
There was a problem hiding this comment.
It'd be helpful to speak to that in the comment.
// Adding indeterminate styles here avoids style conflicts from setting in lines 127-129.I wanted to document the decision here so that if future devs know to not move them in case they miss the radio:indeterminate regressions as we did in early testing.
Does that mean moving the styles to 127-132 will cause similar regressions with checkbox? If so, we can just change radio → checkbox because it looks like one affects the other.
…wds/uswds into checkbox-indeterminate-feature-branch
Update 3/5/24@mejiaj I updated and resolved the comments I was able to! Here are my responses to your other comments: Was indeterminate radio resolved? Yes! Radio styles now maintain the expected unselected/indeterminate state styles Is indeterminate radio + fix captured in PR description? Yes, it was captured in the in the "Radio Testing" section but I've moved it to "Solution" for better visibility:
Language surrounding Quoted from this comment:
The thing I want to highlight is that the From MDN:
Because of this, I lean towards something like Updated in 2636dbe Let me know if you prefer something else. Mixin SASSDoc comments Added SASSDoc comments for the new mixins. I also renamed them to use Resolved in a399129 Checkbox styles SASSDoc comments I updated the SASSDoc comment to focus on the class rather than the line numbers.
I opted for this because my previous changes caused the desired line range to move and I realized any references to the line numbers could easily be made incorrect. Resolved in c31e6f9 Question about radio regression
No, the regression is specifically found in radio buttons. It was caused due to unchecked radio groups receiving the indeterminate style by default. Therefore, we only want checkboxes to receive these styles to maintain the standard “unchecked” radio styles. Footnotes |
|
Wonderful! I'm excited about this landing in main! |
Summary
Added styles for indeterminate checkboxes. Checkboxes will now display as indeterminate when you set
input.indeterminate = truevia JavaScript or add thedata-indeterminateattribute.Tip
This is only a style addition and does not affect checkbox functionality. Originally contributed by @lpsinger in #5549.
Breaking change
This is not a breaking change.
Related issue
Resolves #918
Related pull requests
Preview link
Default checkbox (w/ indeterminate toggle) →
Indeterminate checkbox →
Indeterminate checkbox tile →
Problem statement
USWDS Checkboxes do not support the
:indeterminateCSS pseudo-class which can be set via JS.More info about indeterminate on MDN Docs
Solution
Add USWDS styles for indeterminate checkboxes.
Note from #5549
Major changes
Mixins were required to
$input-active-colorwithin-checkbox-and-radio-colorsmixinTesting and review
Checkbox testing
Radio testing
Radio groups without a selected item receive the
:indeterminatepseudo-class, which caused visual regressions in early testing.checkedattribute from selected item.Testing checklist