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

Add generic anim-rotate-360 class for use by view_components/spinner. #1251

Merged
merged 3 commits into from
Mar 23, 2021

Conversation

theojulienne
Copy link
Contributor

@theojulienne theojulienne commented Mar 23, 2021

The current implementation of SpinnerComponent in primer/view_components uses an SVG with SMIL animations to render a spinner that spins indefinitely. This unfortunately triggers a bug in Chrome that is very frustrating for users, where a page with SVGs with infinitely-repeating SMIL animations that is placed in the background will trigger a linear task to emit, then run, events for each frame that would have been run, as soon as the page is returned to the foreground. This execution is linear to the number of SVGs with repeating SMIL animations multiplied by the duration the tab is in the background. For GitHub.com webpages, this can take seconds, or even minutes, where the Chrome tab is frozen, or even locking up the entire OS if memory is limited.

This PR is the first of 2 parts, it implements a simple CSS class that rotates an element 360 degrees repeatedly. It is not possible to directly include this inside the SpinnerComponent itself because CSS keyframes seem to need to be included in a full CSS file and not inline styles, and since this was a very simple/generic animation that could easily be reused, it seemed nicest to include this here as a generic anim-rotate-360 class.

The following change becomes possible with this in the CSS, with both being identical, except that the second uses CSS animations which do not trigger this bug:

-  <path d="M15 8a7.002 7.002 0 00-7-7" stroke="currentColor" stroke-width="2" stroke-linecap="round" vector-effect="non-scaling-stroke">
-    <animateTransform attributeName="transform" type="rotate" from="0 8 8" to="360 8 8" dur="1s" repeatCount="indefinite" />
-  </path>
+  <path d="M15 8a7.002 7.002 0 00-7-7" stroke="currentColor" stroke-width="2" stroke-linecap="round" vector-effect="non-scaling-stroke" class="anim-rotate-360" style="transform-origin: 8px 8px;" />

/cc @primer/ds-core
/cc primer/view_components#398 which uses this new class

@vercel
Copy link

vercel bot commented Mar 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-css/mPHsvkXS5vQA7Uk5GwyoZwHFNrkK
✅ Preview: https://primer-css-git-fork-theojulienne-fix-spinner-browser-lag-primer.vercel.app

@simurai
Copy link
Contributor

simurai commented Mar 23, 2021

There is currently .anim-rotate on dotcom that could be used. But it would still be great to move it to Primer CSS. Some considerations:

  • We could keep the .anim-rotate so that we don't need to rename it on dotcom.
  • Would be great to add the new class to the docs.

@vercel vercel bot temporarily deployed to Preview March 23, 2021 08:25 Inactive
@vercel vercel bot temporarily deployed to Preview March 23, 2021 08:32 Inactive
@theojulienne
Copy link
Contributor Author

We could keep the .anim-rotate so that we don't need to rename it on dotcom.

👍 I don't think the 360 added anything.

Would be great to add the new class to the docs.

👍 Added with a rotating mark-github octicon.

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

🙇 Thanks for the updates. Ok, let's get it into 16.0.0 #1241.

@simurai simurai changed the base branch from master to release-16.0.0 March 23, 2021 08:45
@simurai simurai merged commit 7516e96 into primer:release-16.0.0 Mar 23, 2021
@simurai simurai mentioned this pull request Mar 23, 2021
16 tasks
@github-actions github-actions bot mentioned this pull request Mar 26, 2021
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.

2 participants