fix(F0Checkbox): stability audit BLOCKING fixes#3899
Open
eliseo-juan wants to merge 1 commit intofix/stability-shared-checkboxfrom
Open
fix(F0Checkbox): stability audit BLOCKING fixes#3899eliseo-juan wants to merge 1 commit intofix/stability-shared-checkboxfrom
eliseo-juan wants to merge 1 commit intofix/stability-shared-checkboxfrom
Conversation
- Export CheckboxProps interface publicly - Fix StoryObj type to use typeof meta - Add withSnapshot import and Snapshot story - Rename __test__ to __tests__ (convention) - Fix screen import to use @/testing/test-utils - Add tests: indeterminate aria-checked=mixed, required asterisk, name prop acceptance, onCheckedChange false on uncheck
Contributor
There was a problem hiding this comment.
Pull request overview
Applies stability-audit BLOCKING fixes for F0Checkbox, focusing on public typing, Storybook correctness, snapshot coverage, and missing unit tests.
Changes:
- Export
CheckboxPropsfromF0Checkboxfor consumer type-checking. - Fix Storybook
StoryObjtyping and add aSnapshotstory usingwithSnapshot. - Update tests (including
screenimport source) and add new cases for indeterminate/required/name/onCheckedChange behaviors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/react/src/components/F0Checkbox/F0Checkbox.tsx | Exports CheckboxProps publicly (per conventions) but still allows unnamed checkbox rendering. |
| packages/react/src/components/F0Checkbox/tests/F0Checkbox.test.tsx | Adds missing tests and updates imports; some new assertions/selectors are too weak/broad to guarantee correct behavior. |
| packages/react/src/components/F0Checkbox/stories/F0Checkbox.stories.tsx | Fixes StoryObj typing to use typeof meta and adds Snapshot story + withSnapshot import. |
Comments suppressed due to low confidence (3)
packages/react/src/components/F0Checkbox/F0Checkbox.tsx:11
- The PR description/issue states all BLOCKING stability-audit findings are fixed, but
titleis still optional and there’s no alternative accessible name prop. This allows rendering a checkbox with no visible label and noaria-label, which results in an unnamed control for assistive tech. Consider makingtitlerequired, or adding/forwarding an explicitaria-label/aria-labelledbyAPI (and/or a dev-only warning when neither is provided).
export interface CheckboxProps extends DataAttributes {
/**
* The title of the checkbox
*/
title?: string
packages/react/src/components/F0Checkbox/tests/F0Checkbox.test.tsx:189
- This test is labeled as forwarding
requiredto the checkbox element, but it only checks for any[aria-hidden="true"]node in the whole document. That selector is too broad and can pass for unrelated elements, and it doesn’t validate the required indicator is tied to this checkbox. Prefer asserting the*inside the checkbox’s label (e.g., scope withwithin(label)), and/or adjust the test name to reflect that it’s verifying the required asterisk rendering rather than attribute forwarding.
packages/react/src/components/F0Checkbox/tests/F0Checkbox.test.tsx:196 - The
nameprop test currently only asserts that rendering does not throw, so it doesn’t actually cover thatnameis applied/forwarded. Since the underlying checkbox root sets anameattribute, assert it on the rendered element (e.g.,getByRole('checkbox')hasname="my-checkbox") to provide real coverage.
Contributor
📦 Alpha Package Version PublishedUse Use |
Contributor
🔍 Visual review for your branch is published 🔍Here are the links to: |
Contributor
Coverage Report for packages/react
File Coverage
|
||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applies all BLOCKING findings from stability audit issue #3872.
CheckboxPropsinterface publicly so consumers can type-check against itStoryObjtype to usetypeof meta(nottypeof F0Checkbox)withSnapshotimport andSnapshotstory export__test__/→__tests__/to match F0 conventionsscreenimport from@/testing/test-utils(not@testing-library/react)aria-checked="mixed",requiredasterisk visible,nameprop accepted,onCheckedChangecalled withfalseon uncheckRelated
fix/stability-shared-checkboxcontains theui/checkbox.tsxa11y fixes (PR fix(ui/checkbox): stability BLOCKING a11y fixes #3898)Quality gate
pnpm tsc --noEmit✅pnpm lint✅pnpm vitest:ci src/components/F0Checkbox✅ (18/18 passing)