Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Ark UI–backed, slot-based BpkCheckboxV2 implementation alongside the existing checkbox, with theming hooks and Storybook examples to support the rebase.
Changes:
- Added
BpkCheckboxV2slot components (Root,Control,Label,Description, etc.) plus styles, docs, tests, and snapshots. - Added per-dimension theme attributes for V2 (selected color + border radius) and wired exports from
bpk-component-checkbox. - Added Storybook examples for Checkbox V2, ESLint allowlisting for Ark UI checkbox parts, and TypeScript typings for
bpk-theming.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bpk-theming/index.d.ts | Adds TS typings for BpkThemeProvider. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/themeAttributes.ts | Introduces V2 theme attribute arrays for theming dimensions. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/accessibility-test.tsx | Adds jest-axe accessibility coverage for V2 compositions/states. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/snapshots/BpkCheckbox-test.tsx.snap | Stores new V2 render snapshots. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/README.md | Documents V2 usage, states, theming, and slot API. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckboxRoot.tsx | Implements Ark Checkbox.Root wrapper + Backpack data attribute. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckboxLabel.tsx | Implements Ark Checkbox.Label wrapper with Backpack styling. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckboxIndicator.tsx | Provides Indicator slot (currently renders nothing; CSS-driven). |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckboxHiddenInput.tsx | Wraps Ark Checkbox.HiddenInput for native form participation. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckboxDescription.tsx | Adds a description slot styled as secondary text. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckboxControl.tsx | Implements Ark Checkbox.Control wrapper with Backpack styling. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckbox.tsx | Exposes the V2 namespace object of slot components. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckbox.module.scss | Adds V2 styling, state selectors, and themeable properties. |
| packages/bpk-component-checkbox/src/BpkCheckboxV2/BpkCheckbox-test.tsx | Adds unit + interaction tests for V2 slots/root behavior. |
| packages/bpk-component-checkbox/index.ts | Exports BpkCheckboxV2 and new V2 themeAttributes alongside existing exports. |
| examples/bpk-component-checkbox-v2/stories.tsx | Adds Storybook stories for V2 compositions and visual tests. |
| examples/bpk-component-checkbox-v2/examples.tsx | Provides Storybook example compositions including themed variants. |
| .eslintrc | Allows className on Ark UI checkbox parts used by wrappers. |
| .claude/skills/backpack-ark-ui-component/SKILL.md | Adds internal guidance for building Ark UI–backed Backpack V2 components. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
|
||
| <BpkCheckbox.Root | ||
| checked={checked} | ||
| onCheckedChange={({ checked: next }) => setChecked(next)} |
There was a problem hiding this comment.
The README’s controlled usage example uses the wrong onCheckedChange signature. BpkCheckbox.Root calls onCheckedChange?.(details.checked), so consumers receive a CheckedState value (boolean | 'indeterminate'), not an object with a checked property. Update the example to use a single argument (and handle 'indeterminate' if relevant).
| onCheckedChange={({ checked: next }) => setChecked(next)} | |
| onCheckedChange={(next) => setChecked(next === 'indeterminate' ? false : next)} |
| | `Indicator` | `null` | ✓ | Checkmark/dash rendered via CSS — no children needed | | ||
| | `Label` | `<span>` | ✓ | Primary label text; clicking toggles the checkbox | | ||
| | `Description` | `<span>` | — | Secondary/subtitle text below the label | | ||
| | `HiddenInput` | `<input type="hidden">` | ✓ | Native input for form submission | |
There was a problem hiding this comment.
The Slots table says HiddenInput renders as <input type="hidden">, but the implementation uses Ark’s Checkbox.HiddenInput which renders a visually-hidden native <input type="checkbox"> (and snapshots show type="checkbox"). Please correct the rendered element description to avoid misleading consumers.
| | `HiddenInput` | `<input type="hidden">` | ✓ | Native input for form submission | | |
| | `HiddenInput` | `<input type="checkbox">` | ✓ | Visually hidden native checkbox input for form submission | |
|
|
||
| // Invalid / error state — border turns red | ||
| &[data-invalid] { | ||
| border-color: tokens.$bpk-status-danger-spot-day; |
There was a problem hiding this comment.
For the invalid/error state, the border colour token differs from the established checkbox invalid styling in bpk-mixins (bpk-checkbox--invalid uses tokens.$bpk-text-error-day). To keep invalid styling consistent across form controls (and with Checkbox v1), consider switching this to tokens.$bpk-text-error-day (or reusing the same mixin/token choice).
| border-color: tokens.$bpk-status-danger-spot-day; | |
| border-color: tokens.$bpk-text-error-day; |
There was a problem hiding this comment.
They are the same color but the first one might be more suitable from the perspective of naming?
|
|
||
| import BpkCheckbox from './BpkCheckbox'; | ||
|
|
||
| const SimpleCheckbox = ({ ...props }: Record<string, unknown> = {}) => ( |
There was a problem hiding this comment.
SimpleCheckbox collects props as Record<string, unknown> and then spreads them into BpkCheckbox.Root. With strict TypeScript settings, Record<string, unknown> is not assignable to BpkCheckboxRootProps (values are unknown), which can break npm run typecheck. Prefer typing this as Partial<BpkCheckboxRootProps> (or React.ComponentProps<typeof BpkCheckbox.Root>) so the spread is type-safe.
| import BpkCheckbox from './BpkCheckbox'; | |
| const SimpleCheckbox = ({ ...props }: Record<string, unknown> = {}) => ( | |
| import type React from 'react'; | |
| import BpkCheckbox from './BpkCheckbox'; | |
| const SimpleCheckbox = ( | |
| props: Partial<React.ComponentProps<typeof BpkCheckbox.Root>> = {}, | |
| ) => ( |
|
Visit https://backpack.github.io/storybook-prs/4271 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4271 to see this build running in a browser. |
|
|
||
| declare const BpkThemeProvider: (props: BpkThemeProviderProps) => JSX.Element; | ||
|
|
||
| export default BpkThemeProvider; |
There was a problem hiding this comment.
bpk-theming is a plain JS package with no TypeScript source. index.d.ts is a hand-written type declaration file so TypeScript consumers can type-check imports from it. Without it, TypeScript would error with "Could not find a declaration file". We can remove it when BpkThemeProvider transferred to TS
|
Visit https://backpack.github.io/storybook-prs/4271 to see this build running in a browser. |
| justify-content: center; | ||
| align-items: center; | ||
| flex-shrink: 0; | ||
| border: tokens.$bpk-one-pixel-rem * 3 solid tokens.$bpk-text-secondary-day; |
There was a problem hiding this comment.
why not using bpk-border-size-xl: bpk-border-size-xl is 3px and would break the style in zoom visual test
|
Visit https://backpack.github.io/storybook-prs/4271 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4271 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4271 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4271 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4271 to see this build running in a browser. |
https://skyscanner.atlassian.net/browse/CLOV-1055?selectedIssue=CLOV-1328
What
Introduces
BpkCheckboxV2, a composable checkbox component built on Ark UI, co-located in the existingbpk-component-checkboxpackage alongside V1.The V2 component exposes a namespace of slot sub-components (
Root,Control,Indicator,Label,Description,HiddenInput), allowing consumers to compose any label layout without extra props.Why
V1's monolithic API makes it difficult to support flexible label layouts (e.g. title + subtitle, inline links). The composable slot model gives consumers full control over content structure while keeping state management encapsulated in
Root.Changes
New component —
BpkCheckboxV2BpkCheckboxRoot— ArkCheckbox.Rootwrapper; owns checked state, exposeschecked,defaultChecked,onCheckedChange,disabled,invalidpropsBpkCheckboxControl— visual 20×20 checkbox box with BEM stylingBpkCheckboxIndicator— intentionally rendersnull; checkmark and indeterminate dash are handled entirely via CSS onControlBpkCheckboxLabel— primary label text; clicking it toggles the checkbox via ArkBpkCheckboxDescription— secondary/subtitle text slotBpkCheckboxHiddenInput— visually hidden<input type="checkbox">for form submission and accessibilityState styling
checked,indeterminate,disabled,invalid,focus-visible) use Ark UIdata-*attributes as CSS selectors — no JS-driven class toggling@include utils.bpk-focus-indicatorgated on[data-focus-visible]Theming
checkboxSelectedColorThemeAttributes— theme the fill/border colour independentlycheckboxBorderRadiusThemeAttributes— theme the border-radius independentlyBpkThemeProviderwithout requiring bothPackage exports (
bpk-component-checkbox/index.ts)BpkCheckboxV2(namespace object)checkboxSelectedColorThemeAttributes,checkboxBorderRadiusThemeAttributesBpkCheckedState,BpkCheckboxRootProps,BpkCheckboxControlProps,BpkCheckboxLabelProps,BpkCheckboxDescriptionPropsStories
SimpleLabel,TitleAndSubtitle,InlineLinkInLabel,DefaultChecked,Disabled,Indeterminate,Invalid,ThemedVisualTestandVisualTestWithZoomfor PercyNotes
BpkCheckbox) is unchanged; V2 is additive and lives alongside it until V1 is deprecatedindeterminatein V2 is a genuineCheckedStatevalue (via Ark), not a visual-only override as in V1 — this is a behavioural difference consumers should be aware of when migratingcheckboxSelectedColor(V2) replacescheckboxCheckedColor(V1)Remember to include the following changes:
[Clover-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.md