Skip to content

fix: restore visible loading spinner animation#1118

Merged
igboyes merged 2 commits into
mainfrom
igboyes/vir-2613-loading-spinner-animation-is-visually-frozen
Jul 8, 2026
Merged

fix: restore visible loading spinner animation#1118
igboyes merged 2 commits into
mainfrom
igboyes/vir-2613-loading-spinner-animation-is-visually-frozen

Conversation

@igboyes

@igboyes igboyes commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

  • fix the Loader ring rendering as a solid, non-spinning circle because tailwind-merge collapsed the transparent bottom border into the same color-conflict group as the solid sides
  • emit per-side border color classes so the transparent bottom survives the merge and the spin animation reads correctly
  • narrow Loader's color prop to the shared six-color palette and fix two call sites passing unsupported values that were silently falling back to gray

The Loader ring rendered as a uniform circle because tailwind-merge put
border-b-transparent and the all-sides color class in the same border-color
conflict group, dropping the transparent bottom. Emit per-side color classes
(border-t/border-x) so the transparent bottom survives the merge.

Also tighten the color prop to the shared six-color palette, fixing two call
sites that passed unsupported values (white, primary) and silently fell back
to gray.
@linear-code

linear-code Bot commented Jul 8, 2026

Copy link
Copy Markdown

VIR-2613

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • Now that Loader.color is a narrowed union without a fallback, it might be worth adding a development-time safeguard (e.g., satisfies Record<LoaderColor, string> or an exhaustive check) so future additions to LoaderColor can’t accidentally be missed in colorToClass.
  • Consider extracting the color/class mapping used in the test into a shared constant (or reusing colorToClass) so the test asserts behavior rather than duplicating the specific Tailwind class strings, making future palette changes less brittle.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Now that `Loader.color` is a narrowed union without a fallback, it might be worth adding a development-time safeguard (e.g., `satisfies Record<LoaderColor, string>` or an exhaustive check) so future additions to `LoaderColor` can’t accidentally be missed in `colorToClass`.
- Consider extracting the color/class mapping used in the test into a shared constant (or reusing `colorToClass`) so the test asserts behavior rather than duplicating the specific Tailwind class strings, making future palette changes less brittle.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Avoids duplicating the Tailwind class strings in the test so palette
changes to Loader don't require updating hardcoded expectations.
@igboyes

igboyes commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Addressed in 2264bb4: colorToClass is now exported and the test derives its expected classes from it instead of duplicating the Tailwind strings, so palette changes stay in sync automatically.

On the exhaustiveness point: colorToClass is already typed as Record<LoaderColor, string> with a literal object, so TypeScript already errors on any missing or extra key — no further safeguard needed.

@igboyes igboyes merged commit 8b1abf5 into main Jul 8, 2026
10 checks passed
@igboyes igboyes deleted the igboyes/vir-2613-loading-spinner-animation-is-visually-frozen branch July 8, 2026 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant