Skip to content

chore: 🤖 initial css modules setup#810

Open
punkbit wants to merge 38 commits intomainfrom
chore/init-css-modules-setup
Open

chore: 🤖 initial css modules setup#810
punkbit wants to merge 38 commits intomainfrom
chore/init-css-modules-setup

Conversation

@punkbit
Copy link
Collaborator

@punkbit punkbit commented Feb 4, 2026

Why?

Provide the initial setup foundations for CSS Modules for migrating from Styled-Components in a sustainable and iterative manner.

This library will now use CSS Modules for styling and is distributed unbundled, giving your application full control over bundling and optimisations. This means you only include what you actually use, resulting in smaller bundle sizes and better performance!

⚠️ WARNING: Depends on #773 and #784; these should be merged first.

How?

  • Process static CSS imports
  • Stores raw stylesheets in distribution
  • Makes distributed components import stylesheets from distribution
  • Provide documentation

Preview?

demo-button-ss-to-css-migration-states.mov

Demonstrates computed CSS Module in DOM

Screenshot 2026-02-04 at 18 26 37

Distribution showcasing Button.module.css (in-place)

dist
├── cjs
│   └── components
│       └── Button
│           ├── Button.cjs
│           ├── Button.cjs.map
│           └── Button.module.css
├── esm
│   └── components
│       └── Button
│           ├── Button.js
│           ├── Button.js.map
│           └── Button.module.css
└── types
    └── components
        └── Button
            └── Button.d.ts

@vercel
Copy link

vercel bot commented Feb 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
click-ui Ready Ready Preview, Comment Feb 4, 2026 7:27pm

Request Review

@changeset-bot
Copy link

changeset-bot bot commented Feb 4, 2026

🦋 Changeset detected

Latest commit: 60931e7

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

Base automatically changed from chore/distribution-unbundled to main February 11, 2026 16:50
@punkbit punkbit force-pushed the chore/init-css-modules-setup branch from 77d9494 to 076d438 Compare March 16, 2026 17:55
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@punkbit punkbit marked this pull request as ready for review March 16, 2026 19:04
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 16, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 17, 2026
@github-actions
Copy link

github-actions bot commented Mar 17, 2026

Claude finished @punkbit's task in 3m 10s —— View job


Code Review — chore: initial CSS modules setup

Great work on this! The architecture is well thought-out — marking .module.css as external in the build, then copying them via viteStaticCopy so that the dist JS files import CSS co-located in the dist tree is a clean solution. The BEM naming, token usage, and prefers-reduced-motion support all look solid. Here are a few things worth addressing before merging.


🚨 Must Fix

  • Button: The type prop shadows the native <button type="submit|reset|button"> attribute (Button.tsx:12). Because type is destructured before ...delegated, consumers can never pass type="submit" for form submission — it will be treated as a visual variant selector and silently dropped. The TODO comment in the code acknowledges this, but since this PR rewrites the component it's a good moment to fix it. Renaming the visual-variant prop to variant would be the breaking change needed (with a migration note). Fix this →

  • Button loading shimmer — missing background-repeat: no-repeat on solid variants: The .button--empty.button--loading::before rule includes background-repeat: no-repeat (Button.module.css:231) but the primary, secondary, and danger shimmer rules do not (lines 218–237). Add it to the other three variants for consistent shimmer rendering. Fix this →


♿ Accessibility

  • Button: aria-disabled={disabled || loading} will render aria-disabled="false" on every enabled button (Button.tsx:77). While technically valid, it pollutes the accessibility tree. Prefer aria-disabled={(disabled || loading) || undefined} so the attribute is omitted entirely when the button is interactive.

💡 Suggestions (non-blocking)

  • Tests: No unit tests were added or updated for the migrated Button. The conventions require co-located tests (Button.test.tsx). Consider adding at minimum a smoke test that the loading state sets aria-busy and disabled, and that className merging works correctly.

  • aria-busy when disabled but not loading: Currently aria-busy={loading} is correct, but aria-busy={false} is emitted on every non-loading button. Same pattern as aria-disabled above — use aria-busy={loading || undefined}.

  • viteStaticCopy rename edge case: The rename function in vite.config.ts (lines 127–135) reassembles the relative path from the absolute srcPath by splitting on /src/. If a consuming project ever names a path segment src, this string-search would produce the wrong sub-path. Storing the glob root as a variable and using a path module for the relative calculation would be safer.

  • Token bundle size: Both tokens-light.css and tokens-dark.css are unconditionally imported in ThemeProvider.tsx (~3,600 lines total). Since the dark tokens are only activated by [data-cui-theme="dark"] (set by ClickUIProvider via setRootThemeAttribute), this is functionally correct. Just worth noting for consumers who only ever use one theme — there's no escape hatch today.


Overall this is a well-structured foundation for the migration. The build pipeline approach (external CSS + static copy) is clever and the changeset with migration notes is appreciated. Resolve the type/variant naming and the shimmer inconsistency and this should be ready. 🙌

  • Gather context and understand PR goals
  • Read CONVENTIONS.md and CLAUDE.md
  • Review Button component changes
  • Review CSS module and token files
  • Review build/vite config changes
  • Review ThemeProvider and preview changes
  • Compile findings

@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

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

Built from commit: 0c6765b7cfa72e60a78d4bebcc68d1c5526e3ffc

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