-
Notifications
You must be signed in to change notification settings - Fork 71
LG-4735 feat: creates LoadingSpinner component #3219
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Creates a new @leafygreen-ui/loading-spinner
package to provide a lightweight SVG-based spinning loading indicator component. This addresses the need to avoid bundling the heavy Lottie animation library used by the existing loading-indicator package.
- Implements SVG-based spinner with CSS animations for better bundle size
- Provides standard size options (xsmall to xlarge) plus custom pixel sizing
- Includes comprehensive testing utilities and Storybook examples
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tools/install/src/ALL_PACKAGES.ts |
Adds the new loading-spinner package to the installation list |
packages/loading-spinner/package.json |
Package configuration with dependencies and build scripts |
packages/loading-spinner/src/LoadingSpinner/LoadingSpinner.tsx |
Main component implementation with SVG rendering |
packages/loading-spinner/src/LoadingSpinner/LoadingSpinner.styles.ts |
CSS-in-JS styles and animations for the spinner |
packages/loading-spinner/src/LoadingSpinner/constants.ts |
Size mappings and helper functions |
packages/loading-spinner/src/testing/getTestUtils.tsx |
Testing utilities for component interaction |
packages/loading-spinner/src/LoadingSpinner.stories.tsx |
Storybook configuration and examples |
packages/loading-indicator/src/index.ts |
Deprecation notice for the existing Spinner component |
.changeset/loading-spinner.md |
Changeset for new package release |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
packages/loading-spinner/src/LoadingSpinner/LoadingSpinner.styles.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Staring at this for too long is dizzying lol.
I think there may be a bit of a timing mismatch. In the lottie spinner, the line always reaches its shortest length at the top of the circle and its longest length I believe at the bottom. If we keep this running for a bit, the shortest point starts to be reached at a point on the circle that moves counter clockwise from the top, indicating that amount of time it takes for the line to expand and collapse isn't exactly the same as the amount of time it takes to go in one loop of the circle.
Screen.Recording.2025-10-13.at.5.31.40.PM.mov
Please correct me if I'm wrong and my eyes are just playing tricks on me! 😅
Ah, good catch! You're right, they are more aligned than I realized. Should be fixed now |
🦋 Changeset detectedLatest commit: fbe7354 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 |
@tsck Do you think this needs a codemod? |
Size Change: +599 B (+0.04%) Total Size: 1.6 MB
ℹ️ View Unchanged
|
Given that we're removing props and changing the name of another ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM!
Kind of nit and possibly inaccurate - is the new spinner just a tad bit slower than the old one? It feels just a bit slower to me, but it could be my eyes tricking me. Not going to block on this, just wanted to raise it.
Yeah I wasn't sure about that either. I was just ballpark-ing off the old one. I can speed it up a touch |
@tsck I noticed that the previous sizes didn't match Figma. Updated those in the latest commit Updates sizing to match Figma |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Sped up a bit also does look better to me!
✍️ Proposed changes
'@leafygreen-ui/loading-indicator': major
LG-4735
The
Spinner
component has been re-written as a lightweight SVG-based spinning loading indicator component, to eliminate the use of Lottie player (a heavy run-time animation library).Importing
Spinner
component from the package root is not recommended, since this will also bundle the heavyPageLoader
component, which still uses Lottie player.For simple loading spinners, prefer importing the spinner directly from
@leafygreen-ui/loading-indicator/spinner
Prop changes:
displayOption
(andsizeOverride
) prop with a singlesize
prop. The size mapping has also been updated to reflect the Figma spec.baseFontSize
propdescription
prop.