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

Add indeterminate prop to the Checkbox #1415

Merged
merged 14 commits into from
May 26, 2023

Conversation

long-lazuli
Copy link
Contributor

@long-lazuli long-lazuli commented Feb 5, 2022

Purpose

Use another symbol when state is indeterminate.

Compliant with design:
image

Approach and changes

I tried to stay as close as possible of the checked state styling implementation.
I add a new SVG in the label for the indeterminate symbol.
And I put a new props on both SVGs to be able to select them with CSS.

There is now indeterminate property.
It takes visually precedence visually over the checked state.
Natively, the indeterminate state of the input would be set to false, when the checked state change.
Here, because it came from a props, it stays on.

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@changeset-bot
Copy link

changeset-bot bot commented Feb 5, 2022

🦋 Changeset detected

Latest commit: 687524b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Feb 5, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/2NB9B8d1nuq3zDyZk1SgvMPcgAhq
✅ Preview: https://oss-circuit-ui-git-feat-checkbox-indeterminate-state-sumup.vercel.app

@codecov
Copy link

codecov bot commented Feb 5, 2022

Codecov Report

Merging #1415 (687524b) into main (503d6e5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1415      +/-   ##
==========================================
+ Coverage   92.08%   92.09%   +0.01%     
==========================================
  Files         170      171       +1     
  Lines        3599     3607       +8     
  Branches     1267     1270       +3     
==========================================
+ Hits         3314     3322       +8     
  Misses        264      264              
  Partials       21       21              
Impacted Files Coverage Δ
...ckages/circuit-ui/components/Checkbox/Checkbox.tsx 88.57% <100.00%> (+2.36%) ⬆️
...rcuit-ui/components/Checkbox/IndeterminateIcon.tsx 100.00% <100.00%> (ø)

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

@long-lazuli thanks a lot for this, and sorry for the wait!

First of all, I like how clean this looks, after our chat on Slack I had assumed the implementation would be very JavaScript-heavy. In contrast this seems very lean 💯

So let's get to it. First, I just wanted to make sure I understand indeterminate and the overall implementation correctly:

The way this would work is that since there is no indeterminate attribute in HTML, developers would manually set the underlying <input type="checkbox"> element's indeterminate property as true, and this would make the SVG appear via the transform.

☝️ is this right?

Overall, I really like the implementation, but I have two main concerns about it:

  1. Clarity—I mentioned this in a comment below: since a checkbox will always have a boolean value when submitted, indeterminate is basically just a UI thing. Is this clear enough to users and developers? Are we sure that this couldn't lead to issues? (e.g. say an indeterminate checkbox is true under the hood, so the merchant doesn't understand that they've essentially checked it unless they explicitly untick it). Generally, wouldn't there be better UX patterns for the issue we're trying to solve here? (I'll follow-up about your use-case on Slack)
  2. Accessibility—this is related to the above. Because this is an uncommon pattern, browser/screenreader pairs will handle the state differently. I've quickly tested with VoiceOver and on Chrome, the checkbox state is announced as "mixed", whereas it's announced on Safari as its actual state ("ticked", i.e. true in the story you created). I believe that this might confuse users and lead to similar issues

VO on Chrome:
Screenshot 2022-02-08 at 14 29 06

VO on Safari:
Screenshot 2022-02-08 at 14 28 25

packages/circuit-ui/components/Checkbox/Checkbox.tsx Outdated Show resolved Hide resolved
@long-lazuli
Copy link
Contributor Author

long-lazuli commented Nov 17, 2022

@long-lazuli long-lazuli force-pushed the feat/checkbox-indeterminate-state branch from d30665f to d7e64c7 Compare November 17, 2022 08:55
@vercel
Copy link

vercel bot commented Nov 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oss-circuit-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 25, 2023 1:55pm

@long-lazuli
Copy link
Contributor Author

long-lazuli commented Nov 17, 2022

@robinmetral
Sorry for being late to answer your concerns about Safari AT. (#1415 (review))

In this PR, we don't make a new indeterminate props or value at all.

The checkbox indeterminate state is done only by javascript, so I think it's up to the developer to use it carefully.
But I added a note on documentation about it. And added a aria-cheked="mixed" into the concerned story.

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Looks nice! I think I finally understand what your approach is 😅 (letting developers set input.indeterminate to toggle these styles). However I think I'd rather allow a new indeterminate prop so developers aren't forced to go get a ref and remember to also pass aria-checked="mixed". What do you think? Is there any reason you didn't want to add a new prop for this?

Also we should add tests for this 🤓 (we can snapshot styles, and if we go for a prop, then we can also cover a11y by asserting that the input should have aria-checked="mixed" and yadda yadda)

@connor-baer
Copy link
Member

connor-baer commented May 24, 2023

@long-lazuli I refactored large parts of the Checkbox component in #2105. I hope you don't mind if I rebase your branch to resolve the conflicts. I'd love to push this across the finish line.

Copy link
Member

@connor-baer connor-baer left a comment

Choose a reason for hiding this comment

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

@long-lazuli I've added a full example implementation to the stories and documented the best practices around indeterminate checkboxes.

Thank you for contributing the implementation! 🙌 I think this is ready to be merged 🚀

@connor-baer connor-baer changed the title style: adds checkbox style for the indeterminate state Add indeterminate prop to the Checkbox May 25, 2023
@connor-baer connor-baer merged commit f2c20b8 into main May 26, 2023
@connor-baer connor-baer deleted the feat/checkbox-indeterminate-state branch May 26, 2023 10:05
@connor-baer connor-baer mentioned this pull request May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants