-
Notifications
You must be signed in to change notification settings - Fork 127
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
Conversation
🦋 Changeset detectedLatest commit: 687524b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/2NB9B8d1nuq3zDyZk1SgvMPcgAhq |
Codecov Report
@@ 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
|
db646f1
to
d30665f
Compare
There was a problem hiding this 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:
- 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) - 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
d30665f
to
d7e64c7
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
d7e64c7
to
92b3b38
Compare
@robinmetral In this PR, we don't make a new The checkbox indeterminate state is done only by javascript, so I think it's up to the developer to use it carefully. |
5b960c5
to
99e855a
Compare
There was a problem hiding this 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)
packages/circuit-ui/components/Checkbox/__snapshots__/Checkbox.spec.tsx.snap
Outdated
Show resolved
Hide resolved
packages/circuit-ui/components/Checkbox/__snapshots__/Checkbox.spec.tsx.snap
Outdated
Show resolved
Hide resolved
a6d9ae0
to
cb2258c
Compare
@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. |
cb2258c
to
9b3aca7
Compare
There was a problem hiding this 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 🚀
Purpose
Use another symbol when state is indeterminate.
Compliant with design:
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