feat!: split apart checkbox checked and indeterminate props #1520

ahuth · 2023-03-02T19:11:07Z

EFI-949

Summary:

Breaking change! (although I'm not aware of any consumers affected by it)

Give the Checkbox component separate checked and indeterminate props.
Also makes indeterminate checkboxes officially indeterminate in the DOM.

Why:

Thought about this while working on #1511.

https://html.spec.whatwg.org/multipage/input.html#checkbox-state-(type=checkbox) calls out indeterminate as a separate piece of state than checked.

The control is never a true tri-state control, even if the element's indeterminate IDL attribute is set to true. The indeterminate IDL attribute only gives the appearance of a third state.

Practically this means a checkbox can be in all 4 combinations of checked and indeterminate (although there are only 3 visual states, because both indeterminates look the same).

Verified this is how browsers behave with https://codepen.io/ahuth/pen/MWqmwyv.

Test Plan:

Wrote automated tests
CI tests / new tests are not applicable
Manually tested my changes, but I want to keep the details secret
Manually tested my changes, and here are the details:
- Storybook

ahuth · 2023-03-02T19:11:56Z

src/components/Checkbox/__snapshots__/Checkbox.test.tsx.snap

@@ -137,8 +137,6 @@ exports[`<Checkbox /> Disabled story renders snapshot 1`] = `
 class="checkbox"
 >
 <input
- aria-checked="mixed"


Don't need aria-checked when we set indeterminate on the DOM node.

ahuth · 2023-03-02T19:13:30Z

src/components/CheckboxInput/CheckboxInput.tsx

- const checkedProps =
- checked === 'indeterminate'
- ? {
- 'aria-checked': 'mixed' as const,


Previously we set aria-checked so screen readers knew about it, but the DOM itself didn't consider the checkbox to be indeterminate.

Here we fix that (which unfortunately requires JS and jumping through a hoop).

src/components/CheckboxInput/CheckboxInput.tsx

ahuth · 2023-03-02T19:17:24Z

src/components/CheckboxInput/CheckboxInput.tsx

+>(({ checked, className, disabled, indeterminate, ...other }, ref) => {
+ const forwardedRef = useForwardedRef(ref);
+
+ // Make this checkbox indeterminate. Can only be done with JS for some reason.


See facebook/react#1798 for why React hasn't added an indeterminate prop to input checkboxes

TL;DR: Browsers don't have an indeterminate attribute.

codecov · 2023-03-02T19:49:59Z

Codecov Report

Merging #1520 (6fc50f5) into next (6fc50f5) will not change coverage.
The diff coverage is n/a.

❗ Current head 6fc50f5 differs from pull request most recent head 0826bfe. Consider uploading reports for the commit 0826bfe to get more accurate results

@@           Coverage Diff           @@
##             next    #1520   +/-   ##
=======================================
  Coverage   91.86%   91.86%           
=======================================
  Files         284      284           
  Lines        4280     4280           
  Branches      790      790           
=======================================
  Hits         3932     3932           
  Misses        323      323           
  Partials       25       25

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ahuth · 2023-03-03T12:53:23Z

How test coverage decrease when I added tests? 🤔

jinlee93 · 2023-03-03T18:49:47Z

How test coverage decrease when I added tests? 🤔

I think coverage is calculated by Hits / Lines and your new tests covered 11 / 12 new lines (91.66%) which technically brought the average down LOL

…hecked` https://html.spec.whatwg.org/multipage/input.html#checkbox-state-(type=checkbox) considers indeterminate to be a separate thing from a checkbox's checked state. In fact, it calls out that "The indeterminate IDL attribute only gives the appearance of a third state." Basically you can end up with all 4 combinatinos of checked and indeterminate, although visually there are only 3 states (both indeterminate states look the same). We can also see clearly at https://codepen.io/ahuth/pen/MWqmwyv that browsers behave this way.

github-actions · 2023-03-06T12:33:08Z

size-limit report 📦

Path	Size
components	121.2 KB (+0.15% 🔺)
styles	3.1 KB (0%)

ahuth · 2023-03-06T19:02:55Z

@chanzuckerberg/edu-frontend-infra

booc0mtaco · 2023-03-06T21:50:55Z

@ahuth can you update the PR title to be semver? looks like github made the checkbox PR commit non-compliant on merge

It's annoying that it defaults to the title if there are multiple commits on a PR branch...

ahuth · 2023-03-06T22:04:36Z

Oh, the PR title needs to adhere to conventional-commits? Of course!

Guess I assumed that the defaults for merges were fine. Maybe that's why we used to only allow rebase merges (since commit messages are enforced) 🤔

booc0mtaco · 2023-03-06T22:53:22Z

I might be missing what thing(s) break with this change? is it the place where we try and handle checked="indeterminate"? Most of it looks kinda additive to my eye

booc0mtaco · 2023-03-06T23:02:33Z

Oh, the PR title needs to adhere to conventional-commits? Of course!

Guess I assumed that the defaults for merges were fine. Maybe that's why we used to only allow rebase merges (since commit messages are enforced) 🤔

Yah, probably. It's a REALLY annoying "feature" in GitHub where it does the Right Thing (tm) if the branch has one commit to squash and merge, but this behavior when it has multiple ones. I'd honestly prefer if it left the title field blank when ambiguous but 🤷. That code is good, so we can update changelog manually if we want; no biggie

ahuth · 2023-03-07T00:55:28Z

Updated the title.

what thing(s) break with this change?

The checked prop changed from boolean + "indeterminate" to just boolean, which is a breaking change.

ahuth · 2023-03-07T13:18:25Z

@chanzuckerberg/edu-frontend-infra please take a look when you get a chance 🙏

booc0mtaco

Thanks for patching this up! I think this PR will have enough details for anyone to sort out what changes they should make in the off chance they were trying to use indeterminate directly.

booc0mtaco · 2023-03-07T18:10:08Z

src/components/CheckboxInput/CheckboxInput.tsx

 */
- checked?: boolean | 'indeterminate';
+ checked?: boolean;


The breaking change => reducing the available options

jinlee93 · 2023-03-07T18:16:27Z

src/components/Checkbox/Checkbox.stories.tsx

 <Checkbox {...args} disabled />
 <Checkbox {...args} checked disabled />
- <Checkbox {...args} checked="indeterminate" disabled />
+ <Checkbox {...args} disabled indeterminate />


I like how simple this looks

jinlee93 · 2023-03-07T18:16:59Z

src/components/CheckboxInput/CheckboxInput.test.tsx

+ render(<CheckboxInput id="test" />);
+ const checkbox = screen.getByRole<HTMLInputElement>('checkbox');
+ expect(checkbox.indeterminate).toEqual(false);
+ });


Just also manually tested <Checkbox indeterminate> and then checking it to true and seeing that the input.indeterminate === false

Sweet, thanks!

jinlee93 · 2023-03-07T18:18:22Z

src/util/useForwardedRef.ts

+/**
+ * Safely access a forwarded ref, which may or may not be present.
+ */


ahuth · 2023-03-07T19:36:27Z

Thanks!

## [11.0.0](v10.0.0...v11.0.0) (2023-03-17) ### ⚠ BREAKING CHANGES * add `indeterminate` prop to <Checkbox> that's separate from `checked` (#1520) ### Features * add `indeterminate` prop to <Checkbox> that's separate from `checked` ([#1520](#1520)) ([d8e2cc4](d8e2cc4)) * **LoadingIndicator:** extract and use SVG animation directly ([#1540](#1540)) ([6e315ea](6e315ea)) * **menu:** add Menu.PlainButton as a minimally styled Menu button ([#1516](#1516)) ([8268d8e](8268d8e)) ### Bug Fixes * actually use our shared prettier config ([c98ea51](c98ea51)) * **Avatar:** loosen props for avatar aria-label component ([#1544](#1544)) ([4ab9183](4ab9183)) * markdown story styling ([#1536](#1536)) ([89eba6b](89eba6b))

ahuth commented Mar 2, 2023

View reviewed changes

src/components/CheckboxInput/CheckboxInput.tsx Show resolved Hide resolved

ahuth commented Mar 2, 2023

View reviewed changes

ahuth mentioned this pull request Mar 2, 2023

Checkbox #1511

Merged

ahuth force-pushed the ah-indeterminate-checkbox branch from d5410d0 to ce46a2f Compare March 2, 2023 19:46

ahuth force-pushed the ah-indeterminate-checkbox branch 3 times, most recently from 500e095 to 9e8c01f Compare March 3, 2023 12:42

Base automatically changed from ah-checkbox to next March 6, 2023 12:23

ahuth force-pushed the ah-indeterminate-checkbox branch from 9e8c01f to 0826bfe Compare March 6, 2023 12:29

ahuth marked this pull request as ready for review March 6, 2023 12:30

ahuth requested a review from a team March 6, 2023 12:30

ahuth changed the title ~~Split apart checkbox checked and indeterminate props~~ feat!: split apart checkbox checked and indeterminate props Mar 6, 2023

booc0mtaco approved these changes Mar 7, 2023

View reviewed changes

jinlee93 reviewed Mar 7, 2023

View reviewed changes

jinlee93 approved these changes Mar 7, 2023

View reviewed changes

ahuth merged commit d8e2cc4 into next Mar 7, 2023

ahuth deleted the ah-indeterminate-checkbox branch March 7, 2023 19:36

booc0mtaco mentioned this pull request Mar 17, 2023

Release v11.0.0 #1549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: split apart checkbox checked and indeterminate props #1520

feat!: split apart checkbox checked and indeterminate props #1520

ahuth commented Mar 2, 2023 •

edited

Loading

ahuth Mar 2, 2023

ahuth Mar 2, 2023

ahuth Mar 2, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading

ahuth commented Mar 3, 2023 •

edited

Loading

jinlee93 commented Mar 3, 2023 •

edited

Loading

github-actions bot commented Mar 6, 2023

ahuth commented Mar 6, 2023

booc0mtaco commented Mar 6, 2023

ahuth commented Mar 6, 2023 •

edited

Loading

booc0mtaco commented Mar 6, 2023

booc0mtaco commented Mar 6, 2023

ahuth commented Mar 7, 2023 •

edited

Loading

ahuth commented Mar 7, 2023

booc0mtaco left a comment

booc0mtaco Mar 7, 2023

jinlee93 Mar 7, 2023

jinlee93 Mar 7, 2023 •

edited

Loading

ahuth Mar 7, 2023

jinlee93 Mar 7, 2023

ahuth commented Mar 7, 2023

feat!: split apart checkbox checked and indeterminate props #1520

feat!: split apart checkbox checked and indeterminate props #1520

Conversation

ahuth commented Mar 2, 2023 • edited Loading

Summary:

Why:

Test Plan:

ahuth Mar 2, 2023

Choose a reason for hiding this comment

ahuth Mar 2, 2023

Choose a reason for hiding this comment

ahuth Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

ahuth commented Mar 3, 2023 • edited Loading

jinlee93 commented Mar 3, 2023 • edited Loading

github-actions bot commented Mar 6, 2023

size-limit report 📦

ahuth commented Mar 6, 2023

booc0mtaco commented Mar 6, 2023

ahuth commented Mar 6, 2023 • edited Loading

booc0mtaco commented Mar 6, 2023

booc0mtaco commented Mar 6, 2023

ahuth commented Mar 7, 2023 • edited Loading

ahuth commented Mar 7, 2023

booc0mtaco left a comment

Choose a reason for hiding this comment

booc0mtaco Mar 7, 2023

Choose a reason for hiding this comment

jinlee93 Mar 7, 2023

Choose a reason for hiding this comment

jinlee93 Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

ahuth Mar 7, 2023

Choose a reason for hiding this comment

jinlee93 Mar 7, 2023

Choose a reason for hiding this comment

ahuth commented Mar 7, 2023

ahuth commented Mar 2, 2023 •

edited

Loading

ahuth Mar 2, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading

ahuth commented Mar 3, 2023 •

edited

Loading

jinlee93 commented Mar 3, 2023 •

edited

Loading

ahuth commented Mar 6, 2023 •

edited

Loading

ahuth commented Mar 7, 2023 •

edited

Loading

jinlee93 Mar 7, 2023 •

edited

Loading