Repository navigation
MAPLE-4349: Progress icon in Safari - #3440
Conversation
Update Spinner to avoid < animateTransform> component, Safari did not actually spin :(
version plan created
|
View your CI Pipeline Execution ↗ for commit 2b411d7 ☁️ Nx Cloud last updated this comment at |
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3440 +/- ##
==========================================
- Coverage 91.33% 90.98% -0.35%
==========================================
Files 407 277 -130
Lines 6840 5910 -930
Branches 2226 1969 -257
==========================================
- Hits 6247 5377 -870
+ Misses 584 524 -60
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
LinKCoding
left a comment
LinKCoding
left a comment
There was a problem hiding this comment.
Gave it a twirl and it looks good to me, I left some nits, but lemme know your thoughts on the css() comment and we can disc if need be :)
| size?: number | string; | ||
| }; | ||
|
|
||
| // keyframes used due to lack of support for <animateTransform> component for Safari browser |
There was a problem hiding this comment.
just a nit - but when I was looking into this, MDN says it is supported
https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Element/animateTransform#browser_compatibility
but it really didn't spin 😅. So maybe it's more accurate to say something like:
// Using keyframes since the <animationTransform> element doesn't animate in Safari
There was a problem hiding this comment.
Updated the comment, yours is definitely more accurate.
I was considering if it was even important enough to leave this as a comment, since doesn't get used anywhere else in the repo, no search results. But I figure it's at least clear this way to not re-implement it that way later.
There was a problem hiding this comment.
yea, I had the same thought, better to document it somewhere in case someone else working on animation comes across this edge case.
| to: { transform: 'rotate(360deg)' }, | ||
| }); | ||
|
|
||
| const SpinnerRing = styled.g({ |
There was a problem hiding this comment.
Honestly, somewhat a nit, but for consistency's sake (and in case we ever want to use tokens that could reflect a theme) I'd recommend using css() to wrap around the raw style obj
e.g.
import { css } from '@codecademy/gamut-styles';
// ...
const SpinnerRing = styled.g(
css({
animation: `${rotate} 1s linear infinite`,
transformBox: 'view-box',
transformOrigin: 'center',
'@media (prefers-reduced-motion: reduce)': {
animationDuration: '3s',
},
})
);
There was a problem hiding this comment.
Updated this & rechecked in Chrome and Safari, both worked great!
Thanks for the clear code on that, exactly as described.
Code Review comments Using css gamut-styles to wrap applied spinner CSS Update comment to better reflect the Safari Browser issue
|
🚀 Styleguide deploy preview ready! Preview URL: https://6abe78c9354d736dd215142f--gamut-preview.netlify.app |
|
📬 Published Alpha Packages:
|
Overview
Update Spinner to avoid < animateTransform> component, in Safari it did not actually spin :(
PR Checklist
Testing Instructions
sizeprop to make sure you can still resize the spinner for whatever page you likePR Links and Envs