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(svg): animation alternative #317

Merged
merged 19 commits into from
Mar 8, 2024
Merged

feat(svg): animation alternative #317

merged 19 commits into from
Mar 8, 2024

Conversation

danilowoz
Copy link
Owner

@danilowoz danilowoz commented Mar 5, 2024

This introduces a new approach to animate SVG gradients and work around the new chromium limitation.

BREAKING CHANGE: The keyTimes, gradientTransform and animateBegin options have been removed.

Preview: https://kn8w9s-6006.csb.app/?path=/story/react-content-loader--animate
Close #316

Copy link

codesandbox bot commented Mar 5, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@amoshydra
Copy link

amoshydra commented Mar 6, 2024

Tested in Safari 17.3, the linear gradient does not animate.

In Chrome 122.0.6261.94, this works reasonably well, if not identical.
Edit: This also works reasonably well in Firefox 122

@tienifr
Copy link

tienifr commented Mar 6, 2024

Thanks @danilowoz for the PR

This is working well for me in:

  • Chrome 122.0.6261.112 (Official Build) (arm64)
  • Chrome 122.0.6261.64 on Android
  • Firefox 123.0.1 (on Mac)
  • Microsoft Edge Version 122.0.2365.63 (on Mac)

But it's not animating in:

  • Safari Version 17.2.1 (19617.1.17.11.12) on Mac
  • Safari on iOS 17.3.1

@tienifr
Copy link

tienifr commented Mar 6, 2024

@danilowoz According to https://stackoverflow.com/a/59932642, using animateTransform won't work for IE and Safari.

I think we can detect IE, Safari by userAgent and use animate for those browsers, that way, we can make sure it works well in all browsers. I'd be happy to contribute the changes for this if you feel that's the direction we can go with 👍

There's a proven, battle-tested way to check for Safari browser (both mobile and desktop) here

@amoshydra

This comment was marked as outdated.

@amoshydra

This comment was marked as outdated.

@danilowoz
Copy link
Owner Author

I don't want to rely on the userAgent property because then this component would be client-only. I'm looking for a one-fit-all solution, but I'm running out of ideas :(

@danilowoz
Copy link
Owner Author

Maybe we can go for the animateTransform approach:

  • Safari will support it soon - it's currently in the preview step;
  • Internet Explorer, well, it's a deprecated browser;

https://caniuse.com/?search=animateTransform

@tienifr
Copy link

tienifr commented Mar 6, 2024

Safari will support it soon - it's currently in the preview step;

@danilowoz I think there're certain applications that would need to work well on Safari now.

How about adding an optional useLegacyAnimate prop that will allow using the animate (as currently) instead of animateTransform. Any applications that want to support Safari now will pass useLegacyAnimate as true if they detect the Safari browser. So the logic of detecting browser will reside in the users of the library and not the library itself.

Later once Safari adds support for animateTransform, we can deprecate the useLegacyAnimate prop.

This change will be straight-forward to make and will allow react-content-loader to work on all platforms 👍

@abzokhattab
Copy link

Found this article where in the accepted answer the author was able to make the animateTransform work with safari and chrome

@danilowoz
Copy link
Owner Author

Ok, I got it running on Safari. Turns out, the linearGradient element requires an initial gradientTransform value to trigger the animation. Plus, I misread the caniuse.com/?search=animateTransform table, and actually, Safari fully supports animateTransform already, so we're good to go.

Considering deprecating the following props:

  • keyTimes: I can't see how to support it in this new approach, and to be honest, I have never understood how it works. Anyway this library belongs not only to me, but also to the community.
  • gradientTransform: that's a shame, but I couldn't make it work. This is used to allow for changing the gradient orientation and the animation direction.

Another round of review? I try fixing tests meanwhile.

@abzokhattab
Copy link

Tried Chrome, Firefox, and Safari, and they all work as expected. Great job @danilowoz

@Roeefl
Copy link

Roeefl commented Mar 7, 2024

@danilowoz

feat(svg): animation alternative #317
+14,239 −64,905 

You crazy bro? 🥲 this is a full blown repo refactor XD

@danilowoz
Copy link
Owner Author

Screenshot 2024-03-07 at 13 27 20

The output in NPM will be the same

@Roeefl
Copy link

Roeefl commented Mar 7, 2024

My bad. Just saw a bunch of files, where I was expecting mostly 1 file changed.

Nicely done, great job & thanks for the quick response.

@tienifr
Copy link

tienifr commented Mar 7, 2024

@danilowoz Works well for me in all browsers too, thanks!!

@danilowoz
Copy link
Owner Author

All tests and build passed! I'm going to publish a new major version soon

@danilowoz danilowoz merged commit 1f9577c into master Mar 8, 2024
1 check passed
@danilowoz danilowoz deleted the draft/vibrant-mccarthy branch March 8, 2024 17:46
Copy link

github-actions bot commented Mar 8, 2024

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skeletons animations are not working in the latest Chrome version
5 participants