fix(Button): restore loading state visuals broken by CSS Modules migration#1043
Conversation
…ation Pre-migration the styled-components Button skipped the :disabled background when loading and shipped two distinct shimmer keyframes (200px fixed-width vs 200% fill-width). The CSS Modules migration applied :disabled unconditionally and collapsed the shimmer to a single fill-width keyframe, so a loading danger button rendered gray with a half-frozen gradient instead of light red with a sweeping shimmer. - Qualify .button_<type>:disabled with :not(.button_loading) so the per-type background-color stays under the shimmer overlay while loading. - Reintroduce shimmer-fixed (-200px → 200px) and shimmer-fill (100% → -100%) keyframes, switched on .button_fill. - Set the loading gradient via background-image so the per-type rule no longer clobbers background-size / background-repeat. - Add a FillWidthLoading story and a fill-width loading spec entry, and regenerate the affected snapshots. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 64354e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| } | ||
|
|
||
| @keyframes shimmer { | ||
| @keyframes shimmer-fixed { |
There was a problem hiding this comment.
For clarity, can we call this shimmer-fixed-width? shimmer-fixed reads kind of like it's fixing a problem.
There was a problem hiding this comment.
Renamed to shimmer-fixed-width in 64354e0. Good catch on the "fixing" ambiguity.
| } | ||
| } | ||
|
|
||
| @keyframes shimmer-fill { |
There was a problem hiding this comment.
For clarity, I think this would be better named shimmer-fill-width
There was a problem hiding this comment.
Renamed to shimmer-fill-width in 64354e0 for parallel naming.
| animation: shimmer-fixed 1.5s ease-in-out infinite; | ||
| } | ||
|
|
||
| .button_fill.button_loading::before { |
There was a problem hiding this comment.
in the vein of the other changes, I think this would be clearer named button_fill_width
There was a problem hiding this comment.
Renamed .button_fill → .button_fill_width in 64354e0 (matches the fillWidth prop name). All cva, CSS, and reduced-motion references updated; visual snapshots still pass byte-for-byte since this is a pure CSS-name rename.
Address review feedback from @hoorayimhelping: - .button_fill → .button_fill_width (matches the fillWidth prop) - shimmer-fixed → shimmer-fixed-width (avoids reading like "fixing") - shimmer-fill → shimmer-fill-width (parallel naming) Pure CSS-name rename; visual snapshots unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Storybook Preview Deployed✅ Preview URL: https://click-9gc30bmay-clickhouse.vercel.app Built from commit: |
3 participants
Summary
The CSS Modules migration of
Buttonregressed two pieces of the original loading behavior introduced in #741. The regression landed as part of #931 — chore(css-modules): support local visual regression testing via docker — which bundled the Button-to-CSS-Modules migration together with the initial CSS Modules tooling and Playwright setup, so the snapshots were captured against the already-broken implementation and no pre-migration baseline existed to diff against.The two regressions:
:disabledbackground applied while loading. The original styled-components only emitted the disabledbackground-colorrule when!$loading, so a loading danger button kept its default light-red background under the shimmer. After the migration the disabled rule was unconditional, so loading buttons rendered with the gray disabled background.shimmerFullWidth(100% → -100%, 200% size, repeat) forfillWidth, andshimmerFixedWidth(-200px → 200px, 200px size, no-repeat) for fixed-width. The migration collapsed both into one fill-width-style keyframe.This PR restores both behaviors in
Button.module.css:.button_<type>:disabledrule with:not(.button_loading), so the per-type background-color stays under the shimmer overlay while loading.shimmer-fixedandshimmer-fillkeyframes, switched on.button_fill.background-image(not thebackgroundshorthand) so per-type rules don't clobberbackground-size/background-repeat.prefers-reduced-motionselector to cover both shimmer variants.A new
FillWidthLoadingstory plus a fill-width loading entry in the Playwright spec lock in the second shimmer mode. Snapshots regenerated.Before / after
Danger / Loading(light)Primary / Loading(fill width)Test plan
yarn lint:cssyarn lint:codeyarn test Button(10/10)yarn buildyarn test:visual tests/buttons/button.spec.ts(46/46, snapshots regenerated)Note
Low Risk
Scoped to Button CSS, stories, and visual tests; no API or runtime logic changes beyond styling.
Overview
Restores Button loading visuals that regressed during the CSS Modules migration.
Disabled vs loading:
:disabledstyles for primary, secondary, empty, and danger now use:not(.button_loading), so loading buttons keep their variant default background (e.g. danger stays light red) instead of the gray disabled fill.Shimmer: Brings back two animations—fixed-width (
shimmer-fixed-width) for normal buttons and fill-width (shimmer-fill-width) whenfillWidthis set—via renamedbutton_fill_widthand a dedicated.button_fill_width.button_loading::beforerule. Loading gradients usebackground-imageso size/repeat on the pseudo-element are not reset.Adds a FillWidthLoading Storybook story, Playwright snapshots for fill-width loading (light/dark), and a patch changeset for
@clickhouse/click-ui.Reviewed by Cursor Bugbot for commit 64354e0. Bugbot is set up for automated code reviews on this repo. Configure here.