Skip to content

Conversation

@joshblack
Copy link
Member

@joshblack joshblack commented Nov 6, 2025

Closes https://github.com/github/primer/issues/6105

Uses a technique from Spectrum to drive our Spinner animations so that they are always in sync: https://github.com/adobe/react-spectrum/blob/ab5e6f3dba4235dafab9f81f8b5c506ce5f11230/packages/%40react-spectrum/s2/src/Skeleton.tsx#L21

We build on top of this technique since we want spinners to also work during SSR -> hydration. As a result, this PR adds the following:

  • Use CSS Animations so that spinners work when first rendered to the page
  • When hydrating, JavaScript will find this animation, pause it, and use the start time of that animation to synchronize all existing, and new, spinners to the same cycle

We use the Web Animations API to synchronize these since we cannot do this with CSS alone without introducing jank during the transition between CSS -> JS (at least none that I could find 😅)

Quick video with before/after with these changes:

Screen.Recording.2025-12-08.at.3.10.40.PM.mov

Changelog

New

  • Add story for synchronized spinners demo

Changed

  • Add a feature flag to control a new behavior where spinners are synchronized

Removed

Rollout strategy

  • Minor release

We will roll this out behind a feature flag first and then stabilize as-needed

@changeset-bot
Copy link

changeset-bot bot commented Nov 6, 2025

🦋 Changeset detected

Latest commit: b21ee3c

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

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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

@github-actions github-actions bot added staff Author is a staff member integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Nov 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

@joshblack joshblack marked this pull request as ready for review November 10, 2025 17:23
@joshblack joshblack requested a review from a team as a code owner November 10, 2025 17:23
@github-actions github-actions bot requested a deployment to storybook-preview-7157 November 10, 2025 17:23 Abandoned
@github-actions github-actions bot requested a deployment to storybook-preview-7157 November 10, 2025 17:27 Abandoned
Copy link
Contributor

Copilot AI left a comment

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 updates the Spinner component to synchronize animations across all spinner instances using the Web Animations API instead of CSS animations. This ensures that multiple spinners rendered at different times maintain visual synchronization, creating a more polished user experience.

Key changes:

  • Replaced CSS-based animations with JavaScript Web Animations API to enable synchronization
  • Added useSpinnerAnimation hook that sets startTime to 0 for all spinner instances
  • Maintained accessibility by respecting prefers-reduced-motion preference

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
packages/react/src/Spinner/Spinner.tsx Implements Web Animations API-based animation synchronization via new useSpinnerAnimation hook
packages/react/src/Spinner/Spinner.module.css Removes CSS keyframe animation since animation is now handled in JavaScript
packages/react/src/Spinner/Spinner.examples.stories.tsx Adds SynchronizedSpinners story demonstrating the synchronization feature with delayed spinner mounts
.changeset/afraid-buckets-build.md Documents the change as a minor version update

@siddharthkp
Copy link
Member

@joshblack Did you see the css first implementation with little javascript in https://github.com/github/github-ui/pull/6120, it uses negative animation-delay which I thought was very smart

@joshblack
Copy link
Member Author

joshblack commented Nov 12, 2025

@siddharthkp yeah! I didn't really know if it would synchronize accurately 🤔 When I tried it out and slowed it down they seemed to be off a bit:

Screen.Recording.2025-11-12.at.3.37.56.PM.mov

This could be related to test setup though. Definitely the faster the animation the less I can see it (if at all). I also wasn't sure what would happen with the difference between SSR / client hydration and if the spinner would jump if the delay changes between what is sent in the HTML payload versus hydration

@siddharthkp
Copy link
Member

siddharthkp commented Nov 13, 2025

When I tried it out and slowed it down they seemed to be off a bit

Seems super odd!

Definitely the faster the animation the less I can see it (if at all)

Valid, not sure if it matters in the case of spinners.

I also wasn't sure what would happen with the difference between SSR / client hydration and if the spinner would jump if the delay changes between what is sent in the HTML payload versus hydration

My (untested) guess is that it might start out of sync and then hydrate to be in sync. That would make a really cool demo!

@joshblack
Copy link
Member Author

@siddharthkp let me know if there is anything else you'd like to see here 👀 The MDN docs seem to point out this limitation too:

Note that this is only possible with the Web Animation API: it is impossible to sync two separate animations with CSS animations.
Link

@github-actions github-actions bot requested a deployment to storybook-preview-7157 November 14, 2025 17:50 Abandoned
siddharthkp
siddharthkp previously approved these changes Nov 17, 2025
@siddharthkp siddharthkp dismissed their stale review November 17, 2025 08:59

Had a doubt that I should check first

@siddharthkp
Copy link
Member

@siddharthkp let me know if there is anything else you'd like to see here

Sorry just a question before I approve: Does this work with SSR or does it wait for the client bundle to start animation? If it's the later, then we're making the component worse and better at the same time. 😅 This should ideally act like a progressive enhancement

@joshblack joshblack requested a review from siddharthkp December 8, 2025 19:22
@github-actions github-actions bot requested a deployment to storybook-preview-7157 December 8, 2025 19:30 Abandoned
@joshblack
Copy link
Member Author

Sharing this over from Slack in case folks are curious 👀 Hopefully this does the trick :fingers_crossed:

It gets so weird to not cause jank when going from CSS -> JS 😅 I ended up doing something like:

  • Use CSS for the animation on the server side so spinners keep animating
  • When JS hydrates, pause the existing CSS animation
  • Use the start time of that CSS animation as the start time for all spinners (so they are always in sync)

@primer-integration
Copy link

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/8508

@primer-integration
Copy link

🔬 github-ui Integration Test Results

Check Status Details
CI ❌ Failed View run
Projects (Memex) ❌ Failed View run
VRT ✅ Passed View run

❌ Troubleshooting Failed Checks

CI

This check runs linting, type checking, and unit tests. Check the workflow logs for specific failures.

👉 View detailed logs

Projects (Memex)

This check runs Playwright end-to-end tests for the Projects feature. Check the test artifacts for screenshots and traces.

👉 View detailed logs


Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance.

@joshblack joshblack added this pull request to the merge queue Dec 10, 2025
Merged via the queue into main with commit eddd934 Dec 10, 2025
52 of 54 checks passed
@joshblack joshblack deleted the feat/add-synchronized-spinner-animations branch December 10, 2025 19:07
@primer primer bot mentioned this pull request Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm staff Author is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants