Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(spinner): new spinner variants #4555

Merged
merged 14 commits into from
Jan 30, 2025
Merged

Conversation

Peterl561
Copy link
Contributor

@Peterl561 Peterl561 commented Jan 13, 2025

Closes #

📝 Description

  • default
    • the old existing spinner
  • gradient
    • relies on a custom mask to create the "hole" in the middle of the spinner
  • dots, dots-blink
    • uses different slots than default
  • spinner-bars
    • uses different slots than default
    • implementation is a little questionable, but couldn't find a good way that also would work with user customization through slots

⛳️ Current behavior (updates)

🚀 New behavior

Screen.Recording.2025-01-14.at.00.16.12.mp4

💣 Is this a breaking change (Yes/No):

📝 Additional Information

Summary by CodeRabbit

  • New Features

    • Added multiple spinner variants: default, gradient, wave, dots, and spinner
    • Enhanced Spinner component with new customization options through the spinnerVariant prop
    • Introduced new animation definitions for spinner effects: sway, blink, and fade-out
  • Documentation

    • Updated Spinner component documentation with new variant details and usage examples
    • Added a new section for Spinner variants in the documentation
  • Improvements

    • Expanded styling and configuration options for Spinner component
    • Simplified existing properties for better usability

Copy link

vercel bot commented Jan 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
heroui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 9:33pm
heroui-sb ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 9:33pm
nextui-docs-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 9:33pm
nextui-storybook-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 9:33pm

Copy link
Contributor

coderabbitai bot commented Jan 13, 2025

Warning

Rate limit exceeded

@macci001 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 18 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 26e608d and a4f5c42.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (17)
  • .changeset/clever-pets-arrive.md (1 hunks)
  • apps/docs/content/components/spinner/index.ts (1 hunks)
  • apps/docs/content/components/spinner/variants.raw.jsx (1 hunks)
  • apps/docs/content/components/spinner/variants.ts (1 hunks)
  • apps/docs/content/docs/api-references/heroui-provider.mdx (1 hunks)
  • apps/docs/content/docs/components/spinner.mdx (3 hunks)
  • packages/components/spinner/package.json (1 hunks)
  • packages/components/spinner/src/spinner.tsx (1 hunks)
  • packages/components/spinner/src/use-spinner.ts (3 hunks)
  • packages/components/spinner/stories/spinner.stories.tsx (4 hunks)
  • packages/core/system/src/provider-context.ts (2 hunks)
  • packages/core/system/src/provider.tsx (3 hunks)
  • packages/core/system/src/types.ts (1 hunks)
  • packages/core/theme/src/animations/index.ts (2 hunks)
  • packages/core/theme/src/components/spinner.ts (2 hunks)
  • packages/core/theme/src/utilities/animation.ts (1 hunks)
  • packages/core/theme/src/utilities/index.ts (1 hunks)

Walkthrough

The pull request introduces variants to the Spinner Component, enhancing its functionality with new styles and animations. Key changes include the addition of new spinner variants such as dots, gradient, and wave, updates to the component's theme and animation utilities, and enhancements to the documentation. Multiple files across the project have been modified to incorporate new properties, animations, and visual representations of the Spinner Component.

Changes

File Change Summary
apps/docs/content/components/spinner/index.ts Added variants import to spinnerContent export
apps/docs/content/components/spinner/variants.raw.jsx New App component demonstrating Spinner variants
apps/docs/content/components/spinner/variants.ts Added export for React component mapping
apps/docs/content/docs/components/spinner.mdx Updated documentation with new Variants section and expanded API details
packages/components/spinner/src/spinner.tsx Added variant property and conditional rendering for different spinner styles
packages/components/spinner/src/use-spinner.ts Updated useSpinner to include variant in return object
packages/components/spinner/stories/spinner.stories.tsx Added VariantsTemplate and Variants export to showcase different spinner styles
packages/core/theme/src/animations/index.ts Introduced new animations: sway, blink, and fade-out
packages/core/theme/src/components/spinner.ts Expanded spinner variants, slots, and styling configurations
packages/core/theme/src/utilities/animation.ts Added animation utilities for spinner bar and dot animations
packages/core/theme/src/utilities/index.ts Imported and included animation utilities
packages/components/spinner/package.json Updated peer dependency for @heroui/theme to >=2.4.7
apps/docs/content/docs/api-references/heroui-provider.mdx Added spinnerVariant prop to HeroUIProvider documentation
packages/core/system/src/provider-context.ts Added spinnerVariant to ProviderContextProps type
packages/core/system/src/provider.tsx Introduced spinnerVariant to HeroUIProviderProps interface
packages/core/system/src/types.ts Added SpinnerVariants type definition for spinner variants

Sequence Diagram

sequenceDiagram
    participant User
    participant SpinnerComponent
    participant ThemeUtilities
    participant AnimationUtilities

    User->>SpinnerComponent: Set variant prop
    SpinnerComponent->>ThemeUtilities: Request variant styling
    ThemeUtilities->>AnimationUtilities: Fetch animation details
    AnimationUtilities-->>SpinnerComponent: Return animation configuration
    SpinnerComponent->>User: Render Spinner with selected variant
Loading

Possibly related PRs

  • feat: global labelPlacement prop #4346: This PR introduces a global labelPlacement property that enhances the layout and positioning of labels across various components, which is relevant to the changes made in the main PR regarding the Spinner Component's variants and customization.
  • feat: tailwind variants upgrade #4386: This PR includes updates to the tailwind-variants library and upgrades React to version 18.3.0, which may relate to the overall styling and functionality improvements in the Spinner Component introduced in the main PR.
  • chore(deps): bump RA versions #4611: This PR focuses on updating the upstream versions of React Spectrum components, which may indirectly affect the Spinner Component's integration and compatibility within the broader library ecosystem.

Suggested reviewers

  • wingkwong

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

packages/core/theme/src/components/spinner.ts Outdated Show resolved Hide resolved
"to-primary",
"animate-spinner-linear-spin",
"[animation-duration:1s]",
"[-webkit-mask:radial-gradient(closest-side,rgba(0,0,0,0.0)calc(100%-3px),rgba(0,0,0,1)calc(100%-3px))]",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mask creates the hole in the middle of the gradient circle to give the loading spinner effect

@@ -7,6 +7,35 @@ export interface SpinnerProps extends UseSpinnerProps {}
const Spinner = forwardRef<"div", SpinnerProps>((props, ref) => {
const {slots, classNames, label, getSpinnerProps} = useSpinner({...props});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider refactoring into multiple hooks for different spinner variants since slots are so different

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having one hook should be good as we have this pattern for the rest of the components as well.
Any thought here? @jrgarciadev

Copy link

changeset-bot bot commented Jan 28, 2025

🦋 Changeset detected

Latest commit: a4f5c42

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 19 packages
Name Type
@heroui/spinner Patch
@heroui/theme Patch
@heroui/autocomplete Patch
@heroui/button Patch
@heroui/select Patch
@heroui/table Patch
@heroui/react Patch
@heroui/alert Patch
@heroui/calendar Patch
@heroui/checkbox Patch
@heroui/dropdown Patch
@heroui/form Patch
@heroui/input-otp Patch
@heroui/popover Patch
@heroui/radio Patch
@heroui/snippet Patch
@heroui/date-picker Patch
@heroui/date-input Patch
@heroui/input 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (12)
packages/core/theme/src/components/spinner.ts (4)

25-25: Consider consolidating the bars slot into a single string.
Having the class definitions spread across an array can be less readable and more error-prone. A single string (or a smaller subset of strings) can improve maintainability and clarity if no dynamic logic is required for bars.

Also applies to: 27-27, 29-33


64-65: Ensure consistent color handling for dots and bars.
Defining bg-* classes for each color is clear but somewhat repetitive. Consider extracting these definitions into a shared utility or applying color classes programmatically to prevent duplication and reduce potential mismatches.

Also applies to: 70-71, 76-77, 82-83, 88-89, 94-95, 100-101, 106-107


161-164: Confirm positional updates for the dots variant.
Using translate-y-3/4 for wrapper might offset the spinner too far from its surrounding context. Please confirm the intended vertical positioning or allow a configurable offset so users can adjust easily.


178-229: Consider programmatic generation for compoundVariants.
You have multiple items for each size in both dots and dots-blink. Generating these compound variants programmatically can reduce repetition, simplify maintenance, and lower the risk of typos.

packages/core/theme/src/utilities/animation.ts (2)

3-6: Document the animation timing calculations.

The bar animation uses magic numbers (-1.2s, 0.1s) for timing and transformation (30deg, 140%). Consider adding comments explaining these specific values and how they affect the animation.


7-12: Standardize animation timing across variants.

The dot animations use different timing bases (250ms vs 200ms) without clear reasoning. Consider standardizing these values or documenting why they differ.

apps/docs/content/components/spinner/variants.raw.jsx (1)

5-14: Extract common props to reduce duplication.

The classNames prop is repeated across all spinner instances. Consider extracting it to a constant:

+ const commonProps = {
+   classNames: { label: "text-primary-400 mt-4" }
+ };

  <div className="flex flex-wrap items-end gap-8">
-   <Spinner classNames={{label: "text-primary-400 mt-4"}} label="default" variant="default" />
+   <Spinner {...commonProps} label="default" variant="default" />
    // Apply similar changes to other instances
  </div>
packages/components/spinner/src/spinner.tsx (2)

8-29: Consider optimizing array creation and improving type safety

  1. Replace [...new Array(3)] with Array.from({length: 3}) for better readability and performance.
  2. Consider using a TypeScript enum or union type for variants instead of string literals.
  3. The wrapper and label code is duplicated across variants.
- const {slots, classNames, label, variant, getSpinnerProps} = useSpinner({...props});
+ type SpinnerVariant = 'default' | 'dots' | 'dots-blink' | 'star';
+ const {slots, classNames, label, variant, getSpinnerProps} = useSpinner<{variant: SpinnerVariant}>({...props});

  if (variant === "dots" || variant === "dots-blink") {
    return (
      <div ref={ref} {...getSpinnerProps()}>
        <div className={slots.wrapper({class: classNames?.wrapper})}>
-         {[...new Array(3)].map((_, index) => (
+         {Array.from({length: 3}).map((_, index) => (

31-50: Optimize star variant rendering and add validation

  1. Consider using CSS transform: rotate() instead of creating 12 separate elements.
  2. Add validation for the bar index to ensure it stays within bounds.
  if (variant === "star") {
    return (
      <div ref={ref} {...getSpinnerProps()}>
        <div className={slots.wrapper({class: classNames?.wrapper})}>
-         {[...new Array(12)].map((_, index) => (
+         {Array.from({length: 12}).map((_, index) => (
            <i
              key={`star-${index}`}
              className={slots.bars({class: classNames?.bars})}
              style={
                {
-               "--bar-index": index,
+               "--bar-index": index % 12, // Ensure index stays within bounds
                } as React.CSSProperties
              }
            />
packages/core/theme/src/animations/index.ts (1)

6-8: Enhance animation configuration and performance

  1. Consider using CSS custom properties for animation timings to allow customization.
  2. Add easing functions for smoother transitions.
  3. Use transform: scale() instead of opacity for better performance.
  animation: {
-   sway: "sway 750ms ease infinite",
-   blink: "blink 1.4s infinite both",
-   "fade-out": "fade-out 1.2s linear 0s infinite normal none running",
+   sway: "sway var(--sway-duration, 750ms) cubic-bezier(0.4, 0, 0.2, 1) infinite",
+   blink: "blink var(--blink-duration, 1.4s) cubic-bezier(0.4, 0, 0.2, 1) infinite both",
+   "fade-out": "fade-out var(--fade-duration, 1.2s) cubic-bezier(0.4, 0, 0.2, 1) infinite",
  },
  keyframes: {
    sway: {
      "0%": {
-       transform: "translate(0px, 0px)",
+       transform: "translateY(0)",
      },
      "50%": {
-       transform: "translate(0px, -150%)",
+       transform: "translateY(-150%)",
      },
      "100%": {
-       transform: "translate(0px, 0px)",
+       transform: "translateY(0)",
      },
    },

Also applies to: 73-102

.changeset/clever-pets-arrive.md (1)

1-6: Enhance changeset description

The description should include:

  1. List of new variants added
  2. Any breaking changes or migration notes
  3. Impact on existing implementations
 ---
 "@heroui/spinner": patch
 "@heroui/theme": patch
 ---
 
-Adding variants to the Spinner Component.
+Adding new variants to the Spinner Component:
+- gradient: Uses custom mask for center hole effect
+- dots & dots-blink: Implements dot-based loading indicators
+- star: Implements rotating bars animation
+
+Note: Existing implementations using custom slots may need updates
+as slot effectiveness now depends on the selected variant.
apps/docs/content/docs/components/spinner.mdx (1)

71-74: Improve slots documentation clarity

Add "the" before "component" and consider adding examples for each slot's usage.

-- **circle1**: The first circle of the spinner. (Effective only when variant is `default` or `gradient`)
-- **circle2**: The second circle of the spinner. (Effective only when variant is `default` or `gradient`)
-- **dots**: Dots of the component. (Effective only when variant is `dots` or `dots-blink`)
-- **bars**: Bars of the component. (Effective only when variant is `bars`)
+- **circle1**: The first circle of the spinner. (Effective only when variant is `default` or `gradient`)
+  Example: `<div className={slots.circle1()}>`
+- **circle2**: The second circle of the spinner. (Effective only when variant is `default` or `gradient`)
+  Example: `<div className={slots.circle2()}>`
+- **dots**: Dots of the spinner component. (Effective only when variant is `dots` or `dots-blink`)
+  Example: `<div className={slots.dots()}>`
+- **bars**: Bars of the spinner component. (Effective only when variant is `bars`)
+  Example: `<div className={slots.bars()}>`
🧰 Tools
🪛 LanguageTool

[uncategorized] ~74-~74: You might be missing the article “the” here.
Context: ... of the component. (Effective only when variant is bars) - label: The label conte...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cddba82 and 46ba46a.

📒 Files selected for processing (12)
  • .changeset/clever-pets-arrive.md (1 hunks)
  • apps/docs/content/components/spinner/index.ts (1 hunks)
  • apps/docs/content/components/spinner/variants.raw.jsx (1 hunks)
  • apps/docs/content/components/spinner/variants.ts (1 hunks)
  • apps/docs/content/docs/components/spinner.mdx (3 hunks)
  • packages/components/spinner/src/spinner.tsx (1 hunks)
  • packages/components/spinner/src/use-spinner.ts (2 hunks)
  • packages/components/spinner/stories/spinner.stories.tsx (2 hunks)
  • packages/core/theme/src/animations/index.ts (2 hunks)
  • packages/core/theme/src/components/spinner.ts (2 hunks)
  • packages/core/theme/src/utilities/animation.ts (1 hunks)
  • packages/core/theme/src/utilities/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • apps/docs/content/components/spinner/variants.ts
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/spinner.mdx

[uncategorized] ~74-~74: You might be missing the article “the” here.
Context: ... of the component. (Effective only when variant is bars) - label: The label conte...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

🔇 Additional comments (8)
packages/core/theme/src/components/spinner.ts (6)

21-25: Check slot naming and usage consistency.
The introduction of label, circle1, circle2, and dots slots adds variety, but please ensure they are used consistently across the component. For instance, confirm that the label slot aligns with user expectations and that no naming conflicts arise if additional slots are added in the future.


42-42: Validate the Tailwind size classes for dots.
The usage of size-1, size-1.5, size-2 may be non-standard unless configured in Tailwind. Confirm that these classes exist in your custom configuration or rename them for consistent sizing (e.g. w-1.5, h-1.5).

Also applies to: 49-49, 56-56


130-147: Review the default variant's border styles and animation durations.
This variant uses both solid and dotted borders, with different animation speeds. Double-check that these visual differences are intentional and that the animations sync up or complement each other for a polished look.


148-160: Check gradient masking for cross-browser compatibility.
The WebKit-specific radial gradient mask may not behave consistently in other browsers. Consider providing a fallback for browsers that do not support -webkit-mask.


169-170: Complete or remove the star variant placeholder.
The star variant is empty, which could confuse maintainers or users. If it is a planned feature, consider adding at least a comment or minimal style to indicate its purpose. Otherwise, remove it to avoid dead code.


176-176: Verify the default variant choice.
Setting "default" as your defaultVariants.variant is likely fine, but ensure it does not introduce unexpected behavior or break existing usage patterns.

packages/core/theme/src/utilities/index.ts (1)

4-10: Validate animation import usage.
With animation now pulled into utilities, confirm that it does not conflict with existing utility keys and that performance remains acceptable. Also, ensure ...animation classes are fully documented if publicly exposed.

apps/docs/content/components/spinner/index.ts (1)

6-14: Ensure documentation covers new variants.
Importing variants into spinnerContent is a good step, but verify that docs clearly describe each new option (dots, dots-blink, gradient, etc.) and provide examples to guide users.

packages/core/theme/src/components/spinner.ts Outdated Show resolved Hide resolved
packages/components/spinner/stories/spinner.stories.tsx Outdated Show resolved Hide resolved
packages/components/spinner/src/use-spinner.ts Outdated Show resolved Hide resolved
packages/components/spinner/src/spinner.tsx Outdated Show resolved Hide resolved
apps/docs/content/docs/components/spinner.mdx Outdated Show resolved Hide resolved
Copy link
Member

@wingkwong wingkwong left a comment

Choose a reason for hiding this comment

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

since the new theme changes will be available in the next release (i.e 2.4.7), so please update @heroui/theme in peerDependencies in packages/components/spinner/package.json. Otherwise, the cli upgrade will not bump theme package when upgrading spinner.

i.e.

- "@heroui/theme": ">=2.4.0"
+ "@heroui/theme": ">=2.4.7"

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/components/spinner/stories/spinner.stories.tsx (1)

52-60: Extract common styles to reduce duplication.

The classNames prop is duplicated across all Spinner instances.

Consider extracting the common styles:

 const VariantsTemplate = () => {
+  const commonClassNames = {label: "text-primary-400 mt-4"};
   return (
     <div className="flex flex-wrap items-end gap-8 py-4">
-      <Spinner classNames={{label: "text-primary-400 mt-4"}} label="default" variant="default" />
-      <Spinner classNames={{label: "text-primary-400 mt-4"}} label="gradient" variant="gradient" />
-      <Spinner classNames={{label: "text-primary-400 mt-4"}} label="star" variant="star" />
-      <Spinner classNames={{label: "text-primary-400 mt-4"}} label="dots" variant="dots" />
+      <Spinner classNames={commonClassNames} label="default" variant="default" />
+      <Spinner classNames={commonClassNames} label="gradient" variant="gradient" />
+      <Spinner classNames={commonClassNames} label="star" variant="star" />
+      <Spinner classNames={commonClassNames} label="dots" variant="dots" />
       <Spinner
-        classNames={{label: "text-primary-400 mt-4"}}
+        classNames={commonClassNames}
         label="dots-blink"
         variant="dots-blink"
       />
     </div>
   );
 };
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46ba46a and acccb82.

📒 Files selected for processing (3)
  • apps/docs/content/docs/components/spinner.mdx (3 hunks)
  • packages/components/spinner/src/use-spinner.ts (2 hunks)
  • packages/components/spinner/stories/spinner.stories.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/docs/content/docs/components/spinner.mdx
  • packages/components/spinner/src/use-spinner.ts
🔇 Additional comments (3)
packages/components/spinner/stories/spinner.stories.tsx (3)

29-34: LGTM! Well-structured variant control.

The variant options are properly defined and align with the PR objectives.


49-63: Consider leveraging Storybook controls for variant demonstration.

The current implementation hardcodes variants instead of utilizing Storybook's controls.


78-85: LGTM! Well-structured story export.

The Variants story follows Storybook's CSF 3.0 format and maintains consistency with other stories.

@macci001 macci001 assigned macci001 and unassigned Peterl561 Jan 30, 2025
@macci001
Copy link
Contributor

@wingkwong updated the package.json

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/components/spinner/stories/spinner.stories.tsx (1)

49-59: LGTM! Consider extracting variant config.

The template effectively showcases all spinner variants. For better maintainability, consider extracting the variant configurations to a constant:

+const SPINNER_VARIANTS = [
+  { label: "default", variant: "default" },
+  { label: "gradient", variant: "gradient" },
+  { label: "star", variant: "star" },
+  { label: "dots", variant: "dots" },
+  { label: "dots-blink", variant: "dots-blink" },
+] as const;

 const VariantsTemplate = (args: SpinnerProps) => {
   return (
     <div className="flex flex-wrap items-end gap-8 py-4">
-      <Spinner {...args} label="default" variant="default" />
-      <Spinner {...args} label="gradient" variant="gradient" />
-      <Spinner {...args} label="star" variant="star" />
-      <Spinner {...args} label="dots" variant="dots" />
-      <Spinner {...args} label="dots-blink" variant="dots-blink" />
+      {SPINNER_VARIANTS.map(({ label, variant }) => (
+        <Spinner key={variant} {...args} label={label} variant={variant} />
+      ))}
     </div>
   );
 };
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e6821d and c7c9714.

📒 Files selected for processing (1)
  • packages/components/spinner/stories/spinner.stories.tsx (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: TypeScript
  • GitHub Check: Build
🔇 Additional comments (2)
packages/components/spinner/stories/spinner.stories.tsx (2)

5-5: LGTM! Well-typed import statement.

The import is correctly typed with SpinnerProps, which is good for maintaining type safety.


74-83: LGTM! Well-structured story export.

The Variants story follows Storybook's Component Story Format (CSF) 3.0 and properly applies styling through classNames.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
packages/components/spinner/src/spinner.tsx (1)

12-27: 🛠️ Refactor suggestion

Reduce code duplication using composition

The variant implementations share common structure (wrapper, elements, label). Consider extracting this into a reusable layout component.

Also applies to: 33-48, 52-58

🧹 Nitpick comments (3)
packages/components/spinner/src/spinner.tsx (1)

14-14: Optimize array creation performance

Creating new arrays on every render is inefficient. Consider memoizing these static arrays.

Apply this diff to optimize the arrays:

+const DOTS_ARRAY = [...new Array(3)];
+const BARS_ARRAY = [...new Array(12)];

 const Spinner = forwardRef<"div", SpinnerProps>((props, ref) => {
   const {slots, classNames, label, variant, getSpinnerProps} = useSpinner({...props});

   if (variant === "wave" || variant === "dots") {
     return (
       <div ref={ref} {...getSpinnerProps()}>
         <div className={slots.wrapper({class: classNames?.wrapper})}>
-          {[...new Array(3)].map((_, index) => (
+          {DOTS_ARRAY.map((_, index) => (
           // ...
   if (variant === "spinner") {
     return (
       <div ref={ref} {...getSpinnerProps()}>
         <div className={slots.wrapper({class: classNames?.wrapper})}>
-          {[...new Array(12)].map((_, index) => (
+          {BARS_ARRAY.map((_, index) => (

Also applies to: 35-35

apps/docs/content/docs/components/spinner.mdx (1)

71-74: Improve clarity in slots documentation.

The slots documentation could be clearer about variant-specific slots.

Consider this structure:

- **circle1**: The first circle of the spinner component. (Effective only when variant is `default` or `gradient`)
- **circle2**: The second circle of the spinner component. (Effective only when variant is `default` or `gradient`)
- **dots**: Dots of the spinner component. (Effective only when variant is `wave` or `dots`)
- **spinnerBars**: Bars of the spinner component. (Effective only when variant is `spinner`)
+ **circle1**: The first circle of the spinner component.
+   - Only used with variants: `default`, `gradient`
+ **circle2**: The second circle of the spinner component.
+   - Only used with variants: `default`, `gradient`
+ **dots**: The dots element of the spinner component.
+   - Only used with variants: `wave`, `dots`
+ **spinnerBars**: The bars element of the spinner component.
+   - Only used with variant: `spinner`
🧰 Tools
🪛 LanguageTool

[uncategorized] ~74-~74: You might be missing the article “the” here.
Context: ...spinner component. (Effective only when variant is spinner) - label: The label co...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

packages/core/theme/src/components/spinner.ts (1)

178-229: Consider consolidating size-specific compound variants.

The compound variants for size-specific styles are repetitive across wave and dots variants.

Consider using a helper function to generate size variants:

const generateSizeVariants = (variant: string) => {
  return ['sm', 'md', 'lg'].map(size => ({
    variant,
    size,
    class: {
      wrapper: size === 'lg' ? 'w-12 h-12' : size === 'md' ? 'w-8 h-8' : 'w-5 h-5'
    }
  }));
};

// Usage in compoundVariants:
...generateSizeVariants('wave'),
...generateSizeVariants('dots'),
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c7c9714 and 26e608d.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (11)
  • apps/docs/content/components/spinner/variants.raw.jsx (1 hunks)
  • apps/docs/content/docs/api-references/heroui-provider.mdx (1 hunks)
  • apps/docs/content/docs/components/spinner.mdx (3 hunks)
  • packages/components/spinner/package.json (1 hunks)
  • packages/components/spinner/src/spinner.tsx (1 hunks)
  • packages/components/spinner/src/use-spinner.ts (3 hunks)
  • packages/components/spinner/stories/spinner.stories.tsx (4 hunks)
  • packages/core/system/src/provider-context.ts (2 hunks)
  • packages/core/system/src/provider.tsx (3 hunks)
  • packages/core/system/src/types.ts (1 hunks)
  • packages/core/theme/src/components/spinner.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/components/spinner/package.json
  • packages/components/spinner/stories/spinner.stories.tsx
  • apps/docs/content/components/spinner/variants.raw.jsx
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/spinner.mdx

[uncategorized] ~74-~74: You might be missing the article “the” here.
Context: ...spinner component. (Effective only when variant is spinner) - label: The label co...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

🔇 Additional comments (7)
packages/core/system/src/provider-context.ts (1)

83-87: LGTM! Well-documented prop addition

The spinnerVariant prop is properly typed and documented, following the established pattern in the codebase.

packages/core/system/src/provider.tsx (1)

65-65: LGTM! Consistent provider implementation

The spinnerVariant prop is properly integrated into the provider's context and dependencies.

Also applies to: 89-89, 98-98

packages/components/spinner/src/use-spinner.ts (2)

72-72: LGTM! Return type updated correctly.

The return type has been updated to include the new variant property.


42-43: 🛠️ Refactor suggestion

Consider adding type safety for variant values.

The variant is extracted from props/context but not validated against allowed values. This could lead to runtime errors if invalid variants are provided.

Add type validation:

+ type SpinnerVariant = 'default' | 'gradient' | 'wave' | 'dots' | 'spinner';

+ const isValidVariant = (variant: string): variant is SpinnerVariant => {
+   return ['default', 'gradient', 'wave', 'dots', 'spinner'].includes(variant);
+ };

  const globalContext = useProviderContext();
- const variant = originalProps?.variant ?? globalContext?.spinnerVariant ?? "default";
+ const providedVariant = originalProps?.variant ?? globalContext?.spinnerVariant;
+ const variant = providedVariant && isValidVariant(providedVariant) 
+   ? providedVariant 
+   : "default";

Likely invalid or redundant comment.

apps/docs/content/docs/components/spinner.mdx (1)

103-108: LGTM! Clear API documentation.

The variant prop is well documented with all possible values.

apps/docs/content/docs/api-references/heroui-provider.mdx (1)

191-196: LGTM! Provider documentation is clear and complete.

The spinnerVariant prop is well documented with its type and possible values.

packages/core/theme/src/components/spinner.ts (1)

165-168: 🛠️ Refactor suggestion

Add reduced motion support for blinking animation.

The dots variant uses blinking animation which may cause discomfort for users with motion sensitivity.

Add reduced motion support:

  dots: {
    wrapper: "translate-y-2/4",
-   dots: ["animate-blink", "spinner-dot-blink-animation"],
+   dots: [
+     "animate-blink",
+     "spinner-dot-blink-animation",
+     "@media (prefers-reduced-motion: reduce) { animation: none; opacity: 0.5; }"
+   ],
  },

Likely invalid or redundant comment.

packages/core/system/src/types.ts Show resolved Hide resolved
packages/components/spinner/src/spinner.tsx Show resolved Hide resolved
@macci001 macci001 force-pushed the feat/v1-spinner-styles branch from 26e608d to 80e23c7 Compare January 30, 2025 13:35
@macci001 macci001 force-pushed the feat/v1-spinner-styles branch from 80e23c7 to a4f5c42 Compare January 30, 2025 13:37
@jrgarciadev jrgarciadev merged commit 1965b84 into canary Jan 30, 2025
6 of 8 checks passed
@jrgarciadev jrgarciadev deleted the feat/v1-spinner-styles branch January 30, 2025 14:22
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.

4 participants