Skip to content

[CLOV-1326] [BpkSegmentedControl] Modernize BpkSegmentedControlV2 composable rebase on Ark-UI#4257

Open
Ezreal Yang (Supremeyh) wants to merge 24 commits intomainfrom
001-composable-segmented-control
Open

[CLOV-1326] [BpkSegmentedControl] Modernize BpkSegmentedControlV2 composable rebase on Ark-UI#4257
Ezreal Yang (Supremeyh) wants to merge 24 commits intomainfrom
001-composable-segmented-control

Conversation

@Supremeyh
Copy link
Contributor

@Supremeyh Ezreal Yang (Supremeyh) commented Mar 4, 2026

Summary

Introduces BpkSegmentedControlV2 as an experimental composable segmented control alongside the existing V1 component (no breaking changes).

The component is rebuilt on Ark-UI SegmentGroup, replacing the buttonContents array API with a dot-notation composable API:

<BpkSegmentedControlV2.Root label="Sort by" value={selected} onChange={setSelected}>
  <BpkSegmentedControlV2.Item value="price">Price</BpkSegmentedControlV2.Item>
  <BpkSegmentedControlV2.Item value="rating">Rating</BpkSegmentedControlV2.Item>
</BpkSegmentedControlV2.Root>

Changes

New component — BpkSegmentedControlV2

packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/

  • BpkSegmentedControlV2.tsx — composable Root + Item. Root uses Children.map to drive Ark-UI SegmentGroup rendering; keyboard navigation (Arrow/Home/End/Space/Enter) and RTL handled internally.
  • BpkSegmentedControlV2.module.scss — CSS module wiring up mixins from bpk-mixins/_segmented-control.scss; item padding overridable via --bpk-segmented-control-padding-y / --bpk-segmented-control-padding-x.
  • common-types.tsBpkSegmentedControlV2RootProps, BpkSegmentedControlV2ItemProps, SegmentTypesV2 type, SEGMENT_TYPES_V2 enum.
  • BpkSegmentedControlV2-test.tsx — 29 assertion-based unit tests across 5 groups: basic rendering, keyboard navigation (automatic & manual activation modes), CSS variable theming, style variants (snapshots), and composable custom content.
  • accessibility-test.tsx — 6 jest-axe tests covering all type variants.
  • BpkSegmentedControlV2.figma.tsx — Figma Code Connect mapping.

New SCSS mixin module — bpk-mixins/_segmented-control.scss

13 new public mixins extracted from the component's styles, making the styling available to non-CSS-module consumers:

Mixin Purpose
bpk-segmented-control-v2 Group container base layout
bpk-segmented-control-v2--canvas-default Canvas default background color
bpk-segmented-control-v2--canvas-contrast Canvas contrast background color
bpk-segmented-control-v2--surface-default Surface default background color
bpk-segmented-control-v2--surface-contrast Surface contrast background color
bpk-segmented-control-v2--shadow Inset box-shadow
bpk-segmented-control-v2__label Visually-hidden accessible label
bpk-segmented-control-v2__item Segment wrapper
bpk-segmented-control-v2__item-control Interactive label (canvas-default colors, focus ring)
bpk-segmented-control-v2__item-control--canvas-contrast Canvas contrast item overrides
bpk-segmented-control-v2__item-control--surface-default Surface default item overrides
bpk-segmented-control-v2__item-control--surface-contrast Surface contrast item overrides
bpk-segmented-control-v2__item-text Inner text/content wrapper

Package & exports

  • index.ts — exports BpkSegmentedControlV2, SEGMENT_TYPES_V2, and all TypeScript types alongside unchanged V1 exports.
  • README.md — V2 usage, props table, CSS custom properties, and V1→V2 migration guide.
  • packages/package.json — adds @ark-ui/react ^5.34.1.

Storybook (examples/bpk-component-segmented-control-v2/)

Stories covering: all 4 type variants, shadow, icon+text, icon-only, RTL, edge cases (2 items / long labels / no initial selection), complex content with icon, complex content with button in slot, and an interactive type-switcher story.

CSS custom properties

Two public properties for consumer padding customisation:

Property Default
--bpk-segmented-control-padding-y bpk-spacing-sm() (0.5rem)
--bpk-segmented-control-padding-x bpk-spacing-md() (1.25rem)

V1 → V2 migration quick-reference

V1 V2
buttonContents={['A', 'B']} Two <BpkSegmentedControlV2.Item> children
selectedIndex={0} value="a"
onItemClick={(i) => fn(i)} onChange={(v) => fn(v)}
type={SEGMENT_TYPES.CanvasDefault} type={SEGMENT_TYPES_V2.CanvasDefault}

Snapshots

Before After
Before After

Testing

Check with figma mcp

SCR-20260304-omjt

Remember to include the following changes:

  • Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [Clover-123][BpkButton] Updating the colour
  • README.md (If you have created a new component)
  • Component README.md
  • Tests
  • Accessibility tests
    • The following checks were performed:
      • Ability to navigate using a keyboard only
      • Zoom functionality (Deque University explanation):
        • The page SHOULD be functional AND readable when only the text is magnified to 200% of its initial size
        • Pages must reflow as zoom increases up to 400% so that content continues to be presented in only one column i.e. Content MUST NOT require scrolling in two directions (both vertically and horizontally)
      • Ability to navigate using a screen reader only
  • Storybook examples created/updated
  • For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

@Supremeyh Ezreal Yang (Supremeyh) added the minor Non breaking change label Mar 4, 2026
Copilot AI review requested due to automatic review settings March 4, 2026 08:53
@Supremeyh Ezreal Yang (Supremeyh) changed the title [CLOV-1326] [BpkSegmentedControl] Make BpkSegmentedControl Composable… [CLOV-1326] [BpkSegmentedControl] Modernize BpkSegmentedControlV2 composable rebase on Ark-UI Mar 4, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces BpkSegmentedControlV2, a new composable segmented control built on Ark-UI SegmentGroup, shipped alongside the existing V1 component (which remains the default export). It also adds supporting specs, tests, Storybook examples, and documentation for the new V2 API.

Changes:

  • Add new BpkSegmentedControlV2 implementation (TS + SCSS) with accessibility/unit tests and Figma Code Connect file.
  • Publish V2 via named exports and document the new API (README + Storybook examples/stories).
  • Add Ark-UI as a dependency and include extensive design/spec documentation under specs/.

Reviewed changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
specs/001-composable-segmented-control/tasks.md Task breakdown for implementing V2 (phases, tests, docs, migration).
specs/001-composable-segmented-control/styling-guide.md Styling and token/CSS-variable guidance for V2.
specs/001-composable-segmented-control/spec.md Product/requirements spec for V2 behavior and constraints.
specs/001-composable-segmented-control/research.md Research notes and decisions (Ark-UI usage, theming approach, etc.).
specs/001-composable-segmented-control/plan.md Implementation plan and file structure for V2 rollout.
specs/001-composable-segmented-control/checklists/requirements.md Spec quality checklist for the feature.
specs/001-composable-segmented-control/api-design.md Proposed API/types/component shape and export strategy.
packages/package.json Adds @ark-ui/react dependency to the published package set.
packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/common-types.ts Defines V2 public types and SEGMENT_TYPES_V2.
packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/accessibility-test.tsx Adds jest-axe accessibility coverage for V2 scenarios.
packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2.tsx Implements Root + Item composable API on Ark SegmentGroup.
packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2.module.scss V2 styling (CSS variables, variants, focus, divider logic).
packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2.figma.tsx Adds Figma Code Connect integration for V2.
packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2-test.tsx Unit tests for V2 rendering, keyboard, variants, and theming.
packages/bpk-component-segmented-control/index.ts Exposes V2 as named exports while keeping V1 default export.
packages/bpk-component-segmented-control/README.md Documents the experimental V2 API and migration notes.
examples/bpk-component-segmented-control-v2/stories.tsx Storybook stories for V2 variants/states.
examples/bpk-component-segmented-control-v2/examples.tsx Story implementations demonstrating V2 usage and edge cases.
examples/bpk-component-segmented-control-v2/examples.module.scss Storybook-specific styling for examples.
.eslintrc Allows Ark-UI SegmentGroup subcomponents to receive className.
Files not reviewed (1)
  • packages/package-lock.json: Language not supported
Comments suppressed due to low confidence (9)

packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2.tsx:109

  • For Home/End (and potentially Arrow keys when only one enabled item exists), onChange can be called even when the newly computed index equals the current index. That can cause redundant onChange calls compared to mouse behaviour. Consider guarding so onChange/selection only fires when the target value differs from the currently focused/selected input.
      case 'Home':
        newIndex = 0;
        break;
      case 'End':
        newIndex = lastIndex;
        break;
      case ' ':
      case 'Enter':
        if (activationMode === 'manual') {
          event.preventDefault();
          onChange?.(inputs[currentIndex].value);
        }
        return;
      default:
        return;
    }

    event.preventDefault();
    inputs[newIndex].focus();
    if (activationMode !== 'manual') {
      onChange?.(inputs[newIndex].value);
    }

packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2-test.tsx:363

  • The US3 theming test only asserts that the wrapper element has a CSS custom property set; it doesn’t verify the component actually consumes the variable (e.g., that the root background or checked item background resolves to it). Please add an assertion on the rendered segmented control element styles (computed style where feasible, or at least presence of the expected var(--bpk-segmented-control-bg) usage on the relevant element).
describe('BpkSegmentedControlV2 — US3: CSS variable theming', () => {
  it('renders root class that CSS variables can be read from', () => {
    const { container } = render(<ThreeItemControl />);
    const root = container.querySelector('[class*="bpk-segmented-control-v2"]');
    expect(root).toBeInTheDocument();
  });

  it('wrapper CSS variable override is applied to the root element context', () => {
    const { container } = render(
      <div style={{ '--bpk-segmented-control-bg': 'red' } as CSSProperties}>
        <ThreeItemControl />
      </div>,
    );
    const wrapper = container.firstElementChild as HTMLElement;
    expect(wrapper.style.getPropertyValue('--bpk-segmented-control-bg')).toBe('red');
  });
});

packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2.module.scss:38

  • The default --bpk-segmented-control-bg token is set to tokens.$bpk-surface-default-day, but V1’s canvas-default uses tokens.$bpk-private-segmented-control-canvas-default-day (and the README/research docs reference the private token). If V2 is meant to be visually indistinguishable from V1 for the same type, the token mapping here likely needs updating (or docs need to be corrected consistently).
.bpk-segmented-control-v2 {
  // Default theme: canvas-default — all CSS variables set from tokens.
  // Consumers (or type variant modifiers) override these to change the theme.
  --bpk-segmented-control-bg: #{tokens.$bpk-surface-default-day};
  --bpk-segmented-control-item-color: #{tokens.$bpk-text-primary-day};
  --bpk-segmented-control-item-disabled-color: #{tokens.$bpk-text-disabled-day};
  --bpk-segmented-control-indicator-bg: #{tokens.$bpk-core-primary-day};
  --bpk-segmented-control-indicator-color: #{tokens.$bpk-text-on-dark-day};
  --bpk-segmented-control-border-radius: #{tokens.$bpk-border-radius-sm};
  --bpk-segmented-control-padding-x: #{tokens.bpk-spacing-base()};
  --bpk-segmented-control-padding-y: #{tokens.bpk-spacing-md()};
  --bpk-segmented-control-divider-color: #{tokens.$bpk-line-day};

packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2.module.scss:23

  • radii and shadows are imported via @use but not referenced anywhere in this stylesheet. Please remove unused Sass modules to keep the file minimal and avoid implying that these mixins are being applied.
@use '../../../bpk-mixins/tokens';
@use '../../../bpk-mixins/utils';
@use '../../../bpk-mixins/typography';
@use '../../../bpk-mixins/radii';
@use '../../../bpk-mixins/shadows';

packages/bpk-component-segmented-control/README.md:166

  • The CSS custom properties table is missing --bpk-segmented-control-shadow, which is part of the public theming API in the specs and is used by the shadow modifier in BpkSegmentedControlV2.module.scss. Please document it here (and ensure the listed default tokens match the actual SCSS defaults, e.g. --bpk-segmented-control-bg).
### CSS custom properties

Override any of these on a wrapper element to theme the component:

| Property | Default | Controls |
|---|---|---|
| `--bpk-segmented-control-bg` | `$bpk-private-segmented-control-canvas-default-day` | Group background |
| `--bpk-segmented-control-item-color` | `$bpk-text-primary-day` | Unselected item text/icon |
| `--bpk-segmented-control-item-disabled-color` | `$bpk-text-disabled-day` | Disabled item text/icon |
| `--bpk-segmented-control-indicator-bg` | `$bpk-core-primary-day` | Selected item background |
| `--bpk-segmented-control-indicator-color` | `$bpk-text-on-dark-day` | Selected item text/icon |
| `--bpk-segmented-control-border-radius` | `$bpk-border-radius-sm` | Group + item corner radius |
| `--bpk-segmented-control-padding-x` | `1rem` | Horizontal item padding |
| `--bpk-segmented-control-padding-y` | `1.25rem` | Vertical item padding |
| `--bpk-segmented-control-divider-color` | `$bpk-line-day` | Divider between items |

packages/bpk-component-segmented-control/src/BpkSegmentedControlV2/BpkSegmentedControlV2-test.tsx:382

  • This parameterised test doesn’t actually assert the specific modifier passed in via expectedClass (it matches any of the variants), so it can pass even if type handling is broken for one variant. Please assert that the root className contains the expected modifier for the current type value.
  it.each([
    [SEGMENT_TYPES_V2.CanvasDefault, 'bpk-segmented-control-v2--canvas-default'],
    [SEGMENT_TYPES_V2.CanvasContrast, 'bpk-segmented-control-v2--canvas-contrast'],
    [SEGMENT_TYPES_V2.SurfaceDefault, 'bpk-segmented-control-v2--surface-default'],
    [SEGMENT_TYPES_V2.SurfaceContrast, 'bpk-segmented-control-v2--surface-contrast'],
  ])('type="%s" adds BEM modifier class "%s"', (type, expectedClass) => {
    const { container } = render(
      <BpkSegmentedControlV2.Root label="Test" type={type}>
        <BpkSegmentedControlV2.Item value="a">A</BpkSegmentedControlV2.Item>
      </BpkSegmentedControlV2.Root>,
    );
    const root = container.firstChild as HTMLElement;
    // CSS modules transform class names; check for a class that includes the modifier
    expect(root.className).toMatch(/canvas-default|canvas-contrast|surface-default|surface-contrast/);
  });

specs/001-composable-segmented-control/styling-guide.md:22

  • This styling guide says shadows is no longer imported (and the shadow is a CSS variable), but the implemented SCSS currently still @uses shadows (and doesn’t reference it). Please update this guide to match the implementation (or adjust the implementation to match the guide).
> `shadows` is no longer imported — the shadow is now a CSS custom property (`--bpk-segmented-control-shadow`) rather than a mixin include, making it consumer-overridable.
>
> Import paths are relative from `src/BpkSegmentedControlV2/` to `packages/bpk-mixins/`.

examples/bpk-component-segmented-control-v2/examples.module.scss:24

  • This stylesheet defines .bpk-component-segmented-control-stories__custom-button, but it isn’t referenced anywhere in the V2 examples/stories. If it’s leftover from an earlier iteration, consider removing it to avoid dead CSS in the Storybook bundle.
.bpk-component-segmented-control-stories__custom-button {
  display: block;
  text-overflow: ellipsis;
  white-space: nowrap;
  overflow: hidden;
}

specs/001-composable-segmented-control/tasks.md:94

  • This tasks doc mentions declaring “all 9 CSS custom properties” but the styling guide/spec define 10 (including --bpk-segmented-control-shadow). Please align the count and ensure the task list matches the agreed public theming API.
  - Apache 2.0 license header
  - Sass imports: `@use '../../../bpk-mixins/tokens'`, `utils`, `typography`, `radii`, `shadows`
  - `.bpk-segmented-control-v2` root: declare all 9 CSS custom properties with Backpack token defaults (per `styling-guide.md` §3–4); `display: grid; grid-auto-columns: 1fr; grid-auto-flow: column; overflow: hidden`; `background-color: var(--bpk-segmented-control-bg)`; `border-radius: var(--bpk-segmented-control-border-radius)`
  - `&--canvas-contrast`, `&--surface-default`, `&--surface-contrast`, `&--shadow` modifier blocks (all CSS variable overrides per `styling-guide.md` §6; shadow uses `@include shadows.bpk-box-shadow-sm`)
  - `.bpk-segmented-control-v2__item`: `display: contents`; `cursor: pointer`; `&[data-disabled]` cursor
  - `.bpk-segmented-control-v2__item-control`: flex centering; `min-height: tokens.bpk-spacing-xl()`; padding from CSS vars; `@include typography.bpk-label-2`; `border-inline-start` divider (logical property); `&[data-state='checked']` selected styles; `&[data-disabled]` disabled styles; `&:focus-visible { @include utils.bpk-focus-indicator }`
  - First/last child border-radius via logical properties: `border-start-start-radius`, `border-end-start-radius`, `border-start-end-radius`, `border-end-end-radius`
  - `.bpk-segmented-control-v2__item-text`: flex + gap; `white-space: nowrap; overflow: hidden; text-overflow: ellipsis; pointer-events: none`
  - **Constitution Check**: `@use` only (no `@import`); all sizing in rem via tokens; BEM with `bpk-` prefix; logical CSS properties for RTL

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

skyscanner-backpack-bot bot commented Mar 4, 2026

Warnings
⚠️

Package source files (e.g. packages/package-name/src/Component.js) were updated, but snapshots weren't. Have you checked that the tests still pass?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 64b9be2

@Supremeyh Ezreal Yang (Supremeyh) added dependencies Pull requests that update a dependency file ai: claude labels Mar 4, 2026
@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@RichardSyq
Copy link
Contributor

Code review

Found 3 issues:

  1. Manual mode Space/Enter does not call inputs[currentIndex].click(), so Ark-UI's internal selection state never updates in uncontrolled usage — the visual indicator stays frozen on the previously selected item. Arrow-key navigation correctly calls inputs[newIndex].click() (lines 108–111) with the comment "Trigger click so Ark-UI updates its internal state for both controlled and uncontrolled usage," but the Space/Enter path only calls onChange?.() without doing the same.

newIndex = lastIndex;
break;
case ' ':
case 'Enter':
if (activationMode === 'manual') {
event.preventDefault();
onChange?.(inputs[currentIndex].value);
}
return;
default:
return;
}
event.preventDefault();
inputs[newIndex].focus();
if (activationMode !== 'manual') {
// Trigger click so Ark-UI updates its internal state for both controlled
// and uncontrolled usage. Ark-UI's onValueChange fires onChange via its
// own listener — do not call onChange manually here to avoid double-firing.
inputs[newIndex].click();
}
};
return (

  1. label prop is typed as optional (label?: string) but passed directly to aria-label={label} with no fallback. When omitted, the role="radiogroup" element has no accessible name — a WCAG 2.2 AA violation (WCAG 4.1.2). The JSDoc on the prop itself says "Required when no visible label is present," yet TypeScript does not enforce this. AGENTS.md requires "All components must meet WCAG 2.2 AA standards" and "Always include proper ARIA labels on interactive components."

* Accessible label for the group. Required when no visible label is present
* in the surrounding layout.
*/
label?: string;
};

  1. The it.each type-variant test uses an overly broad regex (/canvas-default|canvas-contrast|surface-default|surface-contrast/) that matches any of the four variants. The expectedClass parameter passed into each iteration is never used in the assertion, meaning the test would pass even if all four variants applied the same class — it does not verify that the correct modifier is applied for each specific type value.

it.each([
[SEGMENT_TYPES_V2.CanvasDefault, 'bpk-segmented-control-v2--canvas-default'],
[SEGMENT_TYPES_V2.CanvasContrast, 'bpk-segmented-control-v2--canvas-contrast'],
[SEGMENT_TYPES_V2.SurfaceDefault, 'bpk-segmented-control-v2--surface-default'],
[SEGMENT_TYPES_V2.SurfaceContrast, 'bpk-segmented-control-v2--surface-contrast'],
])('type="%s" adds BEM modifier class "%s"', (type, expectedClass) => {
const { container } = render(
<BpkSegmentedControlV2.Root label="Test" type={type}>
<BpkSegmentedControlV2.Item value="a">A</BpkSegmentedControlV2.Item>
</BpkSegmentedControlV2.Root>,
);
const root = container.firstChild as HTMLElement;
// CSS modules transform class names; check for a class that includes the modifier
expect(root.className).toMatch(/canvas-default|canvas-contrast|surface-default|surface-contrast/);
});
it('defaults to canvas-default type when no type provided', () => {
const { container } = render(
<BpkSegmentedControlV2.Root label="Test">
<BpkSegmentedControlV2.Item value="a">A</BpkSegmentedControlV2.Item>
</BpkSegmentedControlV2.Root>,
);
const root = container.firstChild as HTMLElement;

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@Supremeyh
Copy link
Contributor Author

Ezreal Yang (Supremeyh) commented Mar 5, 2026

Code review

Found 3 issues:

thanks your review, address in onChange and label required

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

RichardSyq

This comment was marked as duplicate.

RichardSyq

This comment was marked as duplicate.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

Copy link
Contributor

@RichardSyq Richard-Shen (RichardSyq) left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me review before you hit merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great to see SegmentedControl get the composable treatment. There's a few things we can tighten up before merging!

Thanks for all the effort so far!

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

Copy link
Contributor

@Faye-Xiao Faye (Faye-Xiao) left a comment

Choose a reason for hiding this comment

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

Code review

Found 2 questions:

  1. bpk-segmented-control-v2--canvas-default modifier is a no-op — the base mixin bpk-segmented-control-v2 already initialises --bpk-segmented-control-bg to tokens.$bpk-private-segmented-control-canvas-default-day on line 38. The modifier on lines 63–65 overwrites the same variable with the identical value, making it dead code. Future maintainers will expect this modifier to have an effect and be confused when it doesn't. Either remove the modifier (and document that the default type is always active via the base mixin), or change it to /* inherits base defaults — canvas-default is the component default */ to make the intent explicit.

    @mixin bpk-segmented-control-v2--canvas-default {
    --bpk-segmented-control-bg: #{tokens.$bpk-private-segmented-control-canvas-default-day};
    }

  2. nit: PR description references "disabled states" that no longer exist — the description says the Storybook stories cover "disabled states" but a commit on this branch titled remove disable state removed the feature. There is no disabled prop in BpkSegmentedControlV2ItemProps or BpkSegmentedControlV2RootProps, and no disabled story or example in the stories file. The PR description should be updated to remove the disabled-states claim to avoid misleading reviewers and future readers of the changelog.

    export type BpkSegmentedControlV2RootProps = {
    /**
    * One or more BpkSegmentedControlV2.Item elements.
    */
    children: ReactNode;
    /**
    * Controlled selected value. When provided, onChange must also be provided.
    */
    value?: string;
    /**
    * Initial selected value for uncontrolled usage.
    */
    defaultValue?: string;
    /**
    * Called when the selected segment changes. Receives the value of the newly selected item.
    */
    onChange?: (value: string) => void;
    /**
    * Pre-defined surface theme controlling default token values.
    * @default 'canvas-default'
    */
    type?: SegmentTypesV2;
    /**
    * Applies a box shadow to the group container.
    * @default false
    */
    shadow?: boolean;
    /**
    * Controls whether arrow-key navigation automatically selects the focused item.
    * 'automatic': selection follows focus immediately.
    * 'manual': selection requires an explicit Space or Enter keypress.
    * @default 'automatic'
    */
    activationMode?: 'automatic' | 'manual';
    /**
    * Accessible label for the radiogroup. Always required to satisfy WCAG 4.1.2
    * (the role="radiogroup" element must have an accessible name).
    */
    label: string;
    };
    export type BpkSegmentedControlV2ItemProps = {
    /**
    * Unique identifier for this segment within the group.
    */
    value: string;
    /**
    * Visible content of the segment — text, icons, or a combination.
    */
    children: ReactNode;
    };

🤖 Generated with Claude Code

@Faye-Xiao
Copy link
Contributor

Hi Ezreal Yang (@Supremeyh), I reviewed the examples on the storybook link and found 2 small questions:

  • visual difference: it looks like there is a slight difference in the spacing of the Segemented item
    • Figma spec gives a value of 4px 8px
Screenshot 2026-03-11 at 14 14 25
  • Example screenshots show its space is 8px 4px
Screenshot 2026-03-11 at 14 14 57 Screenshot 2026-03-11 at 14 15 54
  • a11y questions as the screenshot shows
    • Tab index question: I used the Tab key to select the Segmented component, but it only focused on the first item. I suppose it should first focus on the entire SegmentedControl, then move to the first item when I press the Tab key again. And so on, focus one by one.
    • The focus indicator has an extra spacing, as the screenshot shows
Screenshot 2026-03-11 at 14 22 49 - **nit**: Screen reader question: it tells the whole Segmented Item is a radio. I feel like it may not be a right role for screen reader. Screenshot 2026-03-11 at 14 31 11

@Supremeyh
Copy link
Contributor Author

Ezreal Yang (Supremeyh) commented Mar 11, 2026

Hi Ezreal Yang, I reviewed the examples on the storybook link and found 2 small questions:

  1. Good catch,it might due to override, updated.
  2. Tab focus sequence issue - Unreasonable. The current behavior is correct. Your expectation: Tab → Focus on the entire SegmentedControl → Tab again → Focus item by item

But this does not conform to the WAI-ARIA Radio Group specification. The SegmentGroup of Ark-UI is essentially role="radiogroup" + role="radio", and the correct keyboard interaction mode is roving tabindex:

  • Tab → Enter group, focus on the selected option (or the first item)
  • Arrow Left/Right → Move between items
  • Tab again → Exit the group to the next focused element

The container of SegmentedControl itself should not be tab stop. Look at code BpkSegmentedControlV2. TSX: 134, has achieved a complete onKeyDown Arrow/Home/End navigation, this is a standard implementation.

  1. Currently, Segmented Control is in the "value selection" scenario, and the radio semantics are accurate. The screen reader reports that although "radio" does not visually resemble a traditional radio box, so its semantics are correct.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4257 to see this build running in a browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai: claude dependencies Pull requests that update a dependency file minor Non breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants