Skip to content

chore(ButtonGroup): 🤖 Migrate component button group to CSS Modules#986

Open
punkbit wants to merge 14 commits intotest/buttongroup-visual-regressionfrom
chore/migrate-component-button-group-to-css
Open

chore(ButtonGroup): 🤖 Migrate component button group to CSS Modules#986
punkbit wants to merge 14 commits intotest/buttongroup-visual-regressionfrom
chore/migrate-component-button-group-to-css

Conversation

@punkbit
Copy link
Copy Markdown
Contributor

@punkbit punkbit commented Apr 13, 2026

Why?

Migrate the component ButtonGroup from Styled-Components to CSS Modules.

⚠️ Depends on #985 (should merge first)

How?

  • Migrates Styled Components styles onto CSS Modules
  • Use utility CVA for variants
  • Use CSS Variables for tokens

Tickets?

N/A

Contribution checklist?

  • You've done enough research before writing
  • You have reviewed the PR
  • The commit messages are detailed
  • The build command runs locally
  • Assets or static content are linked and stored in the project
  • For documentation, guides or references, you've tested the commands

Preview?

N/A

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 13, 2026

🦋 Changeset detected

Latest commit: 52d9b02

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 Minor

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

@punkbit punkbit force-pushed the chore/migrate-component-button-group-to-css branch from 204a787 to 634aa64 Compare April 13, 2026 14:54
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 13, 2026
punkbit added 2 commits April 13, 2026 16:12
- Rename classes to follow BEM convention (lowercase with underscores)
- Reorder CSS properties alphabetically
- Update component to use new class names
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 13, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 13, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 13, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 13, 2026
@workflow-authentication-public
Copy link
Copy Markdown
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-8ifgcr3y8-clickhouse.vercel.app

Built from commit: e38c0533fb78c343502bb5c79cb279645e1f3970

@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 13, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 13, 2026
@punkbit
Copy link
Copy Markdown
Contributor Author

punkbit commented Apr 13, 2026

  • ButtonGroup.tsx — no React.forwardRef: Per CONVENTIONS.md, components should forward their root element ref. Worth a follow-up to expose the wrapper div ref.

The ButtonGroup is now forwardRef in PR #987. Do ignore!

@punkbit
Copy link
Copy Markdown
Contributor Author

punkbit commented Apr 13, 2026

  • ButtonGroup.module.css lines 12–13, 19–20 — panel padding axes still swapped: This is the third review raising this. The CSS padding shorthand is padding: vertical horizontal. The wrapper rules still put the -x (horizontal) token in the vertical slot...

The issue's already present in the main branch. A new design system Design Tokens is being introduced which will replace all legacy tokens, e.g. #946. Do ignore!

@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 13, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 13, 2026

Claude finished @punkbit's task in 1m 43s —— View job


PR Review: ButtonGroup CSS Modules Migration

Great migration overall — clean removal of Styled Components, solid use of CVA, comprehensive visual regression suite, and excellent accessibility coverage. A few things to flag:


🚨 Must Fix

  • ButtonGroup.module.css lines 77–80 — selected+hover state is invisible: Due to CSS specificity, .button:hover:not(:disabled) (specificity 0,3,0) overrides .button[aria-pressed='true'] (specificity 0,2,0). This means hovering a selected button silently replaces the selected background/color/font with hover styles, making it visually indistinguishable from an unselected hovered button. Users lose the selected state indicator on hover.
    • Fix: add a combined rule so hovered selected buttons retain a visible "selected" appearance, e.g.:
      .button[aria-pressed='true']:hover:not(:disabled) {
        background: var(--click-button-group-color-background-active-hover, var(--click-button-group-color-background-active));
        color: var(--click-button-group-color-text-active);
      }
    Fix this →

💡 Suggestions (non-blocking)

  • ButtonGroup.module.css lines 12–13, 19–20 — CSS class naming deviates from CONVENTIONS.md: The convention file specifies modifiers with values use a hyphenated value: .button_align-center. Here the classes are buttongroup_type_default (underscore-separated), but should be buttongroup_type-default (hyphen for the value part). Minor consistency nit per the project's BEM rules. Fix this →

  • ButtonGroup.module.css line 84 — focus outline uses a background token: outline: 2px solid var(--click-button-group-color-background-active) — ideally focus rings reference a dedicated stroke/focus token rather than a background token. If a proper focus-ring token exists in the design system, it should be used here.


♿ Accessibility

  • All good: aria-pressed on every button, aria-disabled alongside disabled, role="group" with optional aria-label (well-documented in JSDoc), and :focus-visible styles. The Playwright a11y suite is a nice addition.

🧪 Testing

Excellent coverage — unit tests for controlled/uncontrolled/multi-select modes + full visual regression suite for both themes, all variants, hover/focus states. Well done!

@punkbit punkbit marked this pull request as ready for review April 13, 2026 16:17
@punkbit punkbit changed the base branch from main to test/buttongroup-visual-regression April 14, 2026 10:38
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.

1 participant