Skip to content

chore(SplitButton): migrate to CSS Modules with visual regression baseline#1037

Open
Copilot wants to merge 5 commits into
mainfrom
copilot/migrate-split-button-to-css-modules
Open

chore(SplitButton): migrate to CSS Modules with visual regression baseline#1037
Copilot wants to merge 5 commits into
mainfrom
copilot/migrate-split-button-to-css-modules

Conversation

Copy link
Copy Markdown

Copilot AI commented May 20, 2026

Summary

Migrates SplitButton from styled-components to CSS Modules (cva + cn), following the procedure established by ButtonGroup (PR #1034), IconButton (PR #1036), CardPrimary (PR #1038), and CardHorizontal (PR #1039) and codified in the component-css-modules-migration skill.

This branch was generated by Copilot in parallel with the human-authored migrations and arrived as five commits rather than the skill's expected two; the substance is the right shape but the history wasn't squashed. The PR description was a placeholder before this edit.

What landed:

  • test(SplitButton): add visual regression baseline before CSS Modules migration (b248b532) — extends the stories with seven named variants (primary, secondary, disabled-primary, disabled-secondary, fill-width-primary, fill-width-secondary, open-primary) and adds tests/buttons/splitbutton.spec.ts. 16 snapshots captured against the existing styled-components rendering, covering light + dark themes, hover/focus interactive states, the open menu state, and Enter/Space keyboard activation.
  • chore(SplitButton): migrate styling from styled-components to CSS Modules (e0de801d) — replaces five styled-components (SplitButtonTrigger, PrimaryButton, SecondaryButton, ButtonData, DropdownContent) with SplitButton.module.css + cva / cn. The baseline snapshots re-assert byte-for-byte after the migration.
  • chore(SplitButton): address css modules migration review feedback (556d89c2) — replaced !important overrides with a self-compound class selector and tightened a TypeScript widening (as Record<string, unknown>as const). Should be folded into the migration commit before merge.
  • chore(SplitButton): refine css specificity for BaseButton overrides (1a7da13a) — replaced the self-compound selector with a parent-child descendant (.splitbutton__trigger .splitbutton__primary) to beat BaseButton's own padding rules without compounding. Same idea, cleaner expression. Should be folded into the migration commit before merge.
  • chore(SplitButton): apply final review polish (37a37380) — hoists iconWrapperWidthProps to module scope and memoizes the dropdownContentStyle object so the Dropdown.Content style prop is a stable reference. Should be folded into the migration commit before merge.

Judgment calls Copilot made — flagging for reviewer attention

  1. Two styled-component wrappers were dropped, not migrated. The original <ButtonData as={IconWrapper}> and <DropdownContent as={Dropdown.Content}> used styled-components' as polymorphic to render the underlying component (no extra wrapper div in either case). The new code drops the named styled wrappers entirely:

    • <ButtonData><IconWrapper {...{ fillWidth: false }}>. The styled rule was width: auto; — the assumption is that IconWrapper's fillWidth={false} produces the same effect. Worth a manual confirmation that IconWrapper's default-vs-fillWidth=false behavior matches the old width: auto exactly across the snapshotted states.
    • <DropdownContent><Dropdown.Content style={dropdownContentStyle}>. The styled rule was min-width: ${$width}px — replaced with inline style. DOM-equivalent and the open-menu snapshot covers it.
  2. Synthetic interactive variant instead of :not(:disabled) selectors. The trigger's hover/focus-within rules apply only when not disabled. Rather than .splitbutton__trigger:not(.splitbutton__trigger_disabled):hover, Copilot added an interactive cva variant set to !disabled at the call site, so the rule is just .splitbutton__trigger_interactive…:hover. Functionally equivalent; adds one extra class to the DOM when not disabled. A :not() selector would be lower-friction and avoid the extra class — worth deciding which pattern this codebase prefers going forward.

  3. Specificity bump via descendant selectors for nested BaseButton overrides. The padding/border-reset on the inner primary/secondary buttons needs to beat BaseButton's own padding. The first attempt used !important; review pushed back; the second attempt used a self-compound (.splitbutton__primary.splitbutton__primary); the third settled on the descendant .splitbutton__trigger .splitbutton__primary. This is the same class of fix I used in PR chore(CardPrimary): migrate to CSS Modules with visual regression baseline #1038 for CardPrimary's icon size (where Icon's SvgWrapper svg rule was competing); the descendant form is the cleanest.

  4. Drive-by perf optimization in the final commit. The useMemo for dropdownContentStyle and hoisting iconWrapperWidthProps to module scope are micro-optimizations, not styling changes. The migration scope rule treats this as out of scope; it's harmless here but worth a note that future migrations should leave such changes for a follow-up.

  5. Five commits, not two. The skill expects test(...) + chore(...). The three follow-up commits address review feedback and should be squashed before merge so the final history matches the precedent PRs.

Test plan

  • yarn test:visual tests/buttons/splitbutton.spec.ts — all 16 snapshots pass against the baseline with zero regenerations after the migration commit.
  • yarn test SplitButton — unit tests pass unchanged.
  • yarn lint:code src/components/SplitButton/ — no new errors.
  • yarn lint:css — clean.
  • yarn build — succeeds.
  • grep -r 'styled-components' src/components/SplitButton/ — empty.
  • Manual confirmation that IconWrapper fillWidth={false} matches the old <ButtonData> width: auto rendering (see judgment call add a build-tokens file, add missing props to dark mode #1).
  • Squash the three review-feedback commits into the migration commit before merge.

🤖 Generated with Claude Code

…migration

Agent-Logs-Url: https://github.com/ClickHouse/click-ui/sessions/337a1628-053d-459f-af74-329a7b4e5ad3

Co-authored-by: DreaminDani <7872348+DreaminDani@users.noreply.github.com>
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 20, 2026

🦋 Changeset detected

Latest commit: 37a3738

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

Copilot AI requested a review from DreaminDani May 20, 2026 00:55
@DreaminDani DreaminDani changed the title placeholder chore(SplitButton): migrate to CSS Modules with visual regression baseline May 20, 2026
@DreaminDani DreaminDani marked this pull request as ready for review May 20, 2026 01:04
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.

3 participants