Skip to content

chore(ButtonGroup): migrate to CSS Modules with visual regression baseline#1034

Merged
DreaminDani merged 3 commits into
mainfrom
chore/migrate-buttongroup-css-modules
May 15, 2026
Merged

chore(ButtonGroup): migrate to CSS Modules with visual regression baseline#1034
DreaminDani merged 3 commits into
mainfrom
chore/migrate-buttongroup-css-modules

Conversation

@DreaminDani
Copy link
Copy Markdown
Contributor

@DreaminDani DreaminDani commented May 15, 2026

Why?

Combines #985 (visual regression tests) and #986 (CSS Modules migration) into a single PR with a clean commit story. This is the first execution of a pattern we want to reuse for the rest of the CSS Modules migration effort: tests-first commit, then like-for-like styling migration where every snapshot from the first commit still passes byte-for-byte.

Supersedes and closes #985 and #986.

How?

Three commits, each doing one thing:

  1. chore(test:visual): fix Docker recursion and bump playwright to 1.60.0 — prep. The yarn test:visual* scripts weren't working locally (docker-compose services invoked yarn test:visual as their command, recursing through the script and failing inside the container where docker isn't available). Fixed by having services run npx playwright test directly, forwarding extra args from the shell script, bumping @playwright/test from ^1.57.0 to ^1.60.0, and matching the Dockerfile FROM to v1.60.0-noble. Adds --update-snapshots=all (required since Playwright 1.50).

  2. test(ButtonGroup): add visual regression baseline before CSS Modules migration — captures the current styled-components rendering as snapshots. 29 Playwright tests covering light + dark themes, type variants, selection, disabled states, layout, multi-select, hover/focus, and keyboard activation. Stories are extended to one per visual variant. Only other source change is a narrow types tightening (HTMLAttributes<HTMLButtonElement>ButtonHTMLAttributes<HTMLButtonElement>) needed for the new disabled: true option in the test stories to typecheck — pure TypeScript widening, no runtime change.

  3. chore(ButtonGroup): migrate styling from styled-components to CSS Modules — pure styling refactor. Introduces ButtonGroup.module.css and switches the component to cva + cn per the precedent set by Button. Rendered DOM is identical to the previous commit (role="group", role="button", aria-pressed, native disabled, prop spread all preserved). All 29 snapshots from commit 2 pass byte-for-byte with zero regenerations — the strongest possible proof that nothing visible changed. className passthrough is preserved via cn(...) on both the wrapper and each option button. Patch bump; the changeset reads migration of ButtonGroup from styled-components to css modules. no behavior change.

A11y refinements that landed in #985/#986 (adding aria-disabled, removing redundant role="button", plumbing aria-label plumbing with JSDoc, the closest-consumer aria-label in DateTimeRangePicker, :hover:not(:disabled) correctness, :focus-visible styling) are deliberately out of scope here — those are real improvements but they're orthogonal to the migration and would muddy the byte-for-byte guarantee. They can be follow-up PRs on top of this.

Contribution checklist?

  • You've done enough research before writing
  • You have reviewed the PR
  • The commit messages are detailed
  • The build command runs locally
  • yarn test:visual tests/buttons/buttongroup.spec.ts passes (29/29, zero snapshot regenerations between commits 2 and 3)
  • yarn test ButtonGroup passes (15/15)
  • yarn lint:css and yarn lint:code pass with no new errors
  • Assets or static content are linked and stored in the project
  • For documentation, guides or references, you've tested the commands

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 15, 2026

🦋 Changeset detected

Latest commit: be169cb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Patch

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

The yarn test:visual* scripts were unusable because docker-compose
services invoked the same yarn scripts as their container commands,
causing infinite recursion (and failing inside the container where
docker is not available). The Dockerfile CMD had the same bug.

Fixes:
- docker-compose.playwright.yml: services now run npx playwright test
  directly instead of the yarn script.
- Dockerfile.playwright: CMD now invokes npx playwright test directly.
- .scripts/bash/playwright-docker: forwards extra args to playwright
  so callers can scope to specific spec files
  (e.g. yarn test:visual:update tests/buttons/buttongroup.spec.ts).
  Uses --update-snapshots=all since Playwright 1.50+ requires an
  explicit mode.
- Bump @playwright/test from ^1.57.0 to ^1.60.0 and Dockerfile FROM to
  v1.60.0-noble so the playwright runtime and browsers match.
@DreaminDani DreaminDani force-pushed the chore/migrate-buttongroup-css-modules branch from e1a4610 to be169cb Compare May 15, 2026 21:02
@workflow-authentication-public
Copy link
Copy Markdown
Contributor

Storybook Preview Deployed

✅ Preview URL: https://click-48gjdiy1m-clickhouse.vercel.app

Built from commit: 009b04048c55fed918d6d217bf0e79a7895cc54d

@DreaminDani DreaminDani merged commit 61a2856 into main May 15, 2026
8 checks passed
@DreaminDani DreaminDani deleted the chore/migrate-buttongroup-css-modules branch May 15, 2026 21:38
DreaminDani added a commit that referenced this pull request May 15, 2026
Captures the two-commit, byte-for-byte-snapshot pattern executed in
PR #1034 (ButtonGroup) so the same procedure can be applied
mechanically to the remaining components in the CSS Modules migration.

The skill enforces a hard scope rule: the migration commit is a pure
styling refactor — no bundled a11y refinements, type tightening,
consumer updates, or CSS correctness fixes. The visual regression
tests are credible only because the migration commit changes nothing
visible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants