feat(checkbox): add pf-checkbox #2608
Conversation
🦋 Changeset detectedLatest commit: 3634aad 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 |
github-actions
bot
added
demo
✅ Deploy Preview for patternfly-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
@nikkimk any usability concerns here? |
|
@nikkimk ptal when you get a minute |
| } | ||
|
|
||
| override willUpdate() { | ||
| this.#internals.setFormValue(this.checked ? this.value : null); |
There was a problem hiding this comment.
Screenreader is not picking up when checkbox is disabled.
| @@ -0,0 +1,10 @@ | |||
| <pf-checkbox label="Required" required></pf-checkbox> | |||
There was a problem hiding this comment.
Showing as valid for me when I uncheck it.
| @input="${this.#onChange}" | ||
| .disabled="${disabled}" | ||
| .checked="${checked}" | ||
| .indeterminate="${indeterminate}"> |
There was a problem hiding this comment.
Needs an aria-checked value of mixed if indeterminate.
If nested checkboxes, it would need an aria-controls with space-separated IDrefs for each nested checkbox. Not sure how we'll do this without cross-root ARIA unless the component's internals is capturing the IDRefs of the slotted checkboxes.
See https://www.w3.org/WAI/ARIA/apg/patterns/checkbox/examples/checkbox-mixed/#rps_label
There was a problem hiding this comment.
in that case let's defer checkbox until after 3.0, along with listbox
| .to.be.an.instanceOf(PfCheckbox); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Will there be more tests for accessibility, nesting and validation here?
There was a problem hiding this comment.
would like your help to write them based on your insightful feedback above
What I did
Testing Instructions
Notes to Reviewers