Skip to content

chore(Container): migrate to CSS Modules with visual regression baseline#1059

Merged
DreaminDani merged 4 commits into
mainfrom
chore/migrate-container-css-modules
Jun 3, 2026
Merged

chore(Container): migrate to CSS Modules with visual regression baseline#1059
DreaminDani merged 4 commits into
mainfrom
chore/migrate-container-css-modules

Conversation

@DreaminDani

@DreaminDani DreaminDani commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Commits

  • test(Container): add visual regression baseline before CSS Modules migration — adds stories, Playwright spec, and fresh PNG snapshots that capture the current rendering.
  • chore(Container): migrate styling from styled-components to CSS Modules — replaces styled-components with .module.css + cva/cn. DOM-identical, byte-for-byte visual parity verified.

Verification

  • yarn test:visual tests/display/container.spec.ts passes with zero snapshot regenerations (30 snapshots: 15 variants × light/dark)
  • yarn lint:css, yarn lint:code, yarn build all green
  • grep -r 'styled-components' src/components/Container/{Container.tsx,Container.module.css,Container.types.ts,index.ts} empty (the stories harness keeps its GridCenter/Box styled helpers, matching existing migrated components)

Notes

  • Arbitrary-value props (maxWidth, minWidth, maxHeight, minHeight, overflow, display, fillHeight, grow, shrink) are driven through inline CSS custom properties with fallbacks that reproduce the original computed values; enumerable props (gap, padding, orientation, wrap, alignItems, justifyContent, responsiveness) are cva variants.
  • Pre-existing invalid declarations from the source (max-width: auto, flex-direction: auto in the non-responsive media branches) were intentionally preserved as no-ops to keep byte-for-byte parity.

Note

Low Risk
Styling-only refactor of a shared layout component; risk is mitigated by explicit parity goals and broad Playwright visual snapshots, though any missed edge case could affect downstream layouts.

Overview
Container moves off styled-components onto CSS Modules with cva/cn, matching other migrated layout primitives. Token-based gap and padding, flex orientation, wrap, align, and justify are variant classes; sizing, grow/shrink, overflow, and display flow through inline --container-* custom properties. Responsive behavior at 768px is split into container_responsive vs fill/hug non-responsive classes, with comments documenting preserved no-op invalid values from the old implementation for visual parity.

Storybook gains focused variant stories (replacing styled story helpers with inline styles), and Playwright adds light/dark screenshot coverage for 15 layout scenarios. A changeset records a patch release with no intended API or behavior change.

Reviewed by Cursor Bugbot for commit 2031d5b. Bugbot is set up for automated code reviews on this repo. Configure here.

DreaminDani and others added 2 commits June 3, 2026 08:30
…gration

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@DreaminDani DreaminDani self-assigned this Jun 3, 2026
@changeset-bot

changeset-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2031d5b

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 left a comment

Copy link
Copy Markdown
Contributor

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 migrates the Container layout component from styled-components to CSS Modules while preserving rendering and public API behavior, supported by a new Playwright visual regression baseline.

Changes:

  • Add Storybook variants + Playwright visual regression coverage (light globals + dark via prefers-color-scheme) with snapshot baselines.
  • Replace styled-components styling in Container with Container.module.css using cva/cn and inline CSS custom properties for dynamic values.
  • Add a patch changeset documenting the refactor-only migration.

Reviewed changes

Copilot reviewed 5 out of 35 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/display/container.spec.ts Adds visual regression tests for 15 Container variants in light/dark (30 snapshots).
src/components/Container/Container.tsx Migrates component styling to CSS Modules + cva, with dynamic props expressed via CSS variables.
src/components/Container/Container.stories.tsx Adds Storybook variant stories used by the new visual regression suite.
src/components/Container/Container.module.css Introduces CSS Module classes/variants + responsive behavior at the md breakpoint (768px).
.changeset/migrate-container-to-css-modules.md Records the styling migration as a patch change with no intended behavior change.

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

The autodocs source preview rendered the Playground wrapper as
<styled.div>. Swap the GridCenter/Box styled-components for plain divs
with inline styles so the story carries no styled-components usage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@hoorayimhelping hoorayimhelping left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One suggestion.

The stories are way better now!

Comment thread src/components/Container/Container.tsx Outdated
The container's inline style merged CSS custom properties with the
incoming `style` prop into a fresh object on every render. Since
Container is used ubiquitously, wrap it in `useMemo` keyed on the
underlying values so the reference stays stable when nothing changed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@workflow-authentication-public

Copy link
Copy Markdown
Contributor

Storybook Preview Deployed

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

Built from commit: bb1aae2b6d83d35793692553b2371314a79b2f02

@DreaminDani DreaminDani merged commit fb8aed3 into main Jun 3, 2026
10 checks passed
@DreaminDani DreaminDani deleted the chore/migrate-container-css-modules branch June 3, 2026 21:01
DreaminDani added a commit that referenced this pull request Jun 4, 2026
The Icon wrapper built a fresh inline style object of CSS custom
properties on every render. Wrap it in `useMemo` keyed on color/width/
height so the reference stays stable when those haven't changed. Moved
above the early `!Component` return to satisfy rules-of-hooks.

Follow-up to the Container migration review (#1059), which applied the
same stable-reference fix.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DreaminDani added a commit that referenced this pull request Jun 5, 2026
…ules migrations

The CSS Modules migration skill covered emitting layout props as inline CSS
custom properties (and memoizing them) but not that custom properties inherit
by default. Conditionally-emitted ones therefore leak into nested instances of
the same component — the Container regression (#1059, fixed #1067) where a
parent with `fillHeight` stretched every descendant Container to full height.

Add the reset-to-`initial`-with-fallback rule to the .module.css guidance, a
matching pitfall entry, and a checklist gate, and note that visual-regression
snapshots miss this (it only shows when a component nests inside itself), so a
nested-instance computed-style test is required.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DreaminDani added a commit that referenced this pull request Jun 8, 2026
…ties (#1067)

* fix(Container): stop nested Container inheriting layout custom properties

The CSS Modules migration forwards optional layout props (fillHeight, grow,
shrink, minHeight, maxHeight, overflow) to the DOM as CSS custom properties,
emitted inline only when the prop is set. Custom properties inherit by
default, so a nested Container that omits those props inherited its
ancestor's values — e.g. a layout root with `fillHeight` forced
`height: 100%` onto every descendant Container (observed as the control-plane
alert banner stretching to fill the viewport, with knock-on spacing breakage
throughout the page).

Reset these custom properties to `initial` on the base `.container` rule so a
declaration on the element wins over inheritance, and add `var()` fallbacks
matching the styled-components defaults. Verified in a browser: with the bug
a child Container inherited flex-grow:1/height:198px/overflow:hidden; after
the fix it hugs its content (flex-grow:0, height:34px, overflow:visible).

Adds a NestedInheritance story and a computed-style regression test; all 30
existing Container visual snapshots are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* bump version to v0.6.1-rc1

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* docs(skill): warn about inline custom-property inheritance in CSS Modules migrations

The CSS Modules migration skill covered emitting layout props as inline CSS
custom properties (and memoizing them) but not that custom properties inherit
by default. Conditionally-emitted ones therefore leak into nested instances of
the same component — the Container regression (#1059, fixed #1067) where a
parent with `fillHeight` stretched every descendant Container to full height.

Add the reset-to-`initial`-with-fallback rule to the .module.css guidance, a
matching pitfall entry, and a checklist gate, and note that visual-regression
snapshots miss this (it only shows when a component nests inside itself), so a
nested-instance computed-style test is required.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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