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 support for SVG animations #5

Merged
merged 6 commits into from
Oct 30, 2024

Conversation

Trombach
Copy link
Contributor

@Trombach Trombach commented Oct 27, 2024

Description

Adds support for transferring SVG animation state, which is achieved by getting the animation time via SVGSVGElement.getCurrentTime() and setting the time via SVGSVGElement.setCurrentTime().

Todo:

  • decide whether to keep the preliminary svg-anim name
  • add abbreviated name

Tests

Added an animated SVG element to the vt-bag website and tested that the animation state is transferred
Recording

Docs & Examples

vtbag/website#39

@martrapp
Copy link
Contributor

Hey Lukas, thank you so much for this extension!
I'll have a look later today.

martrapp
martrapp previously approved these changes Oct 28, 2024
Copy link
Contributor

@martrapp martrapp left a comment

Choose a reason for hiding this comment

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

Works like a charm!

I've noticed that svg-anim currently has no real use for the key value after the colon.
Do you have an idea for this?

Alternatively, we could use this value to differentiate between CSS animations and SVG animations. An empty key or a value that is not a legal keyframes identifier could mark the SVG case, such as anim:, or anim:/svg But I'm unsure about the DX. Will it be too unintuitive?

I liked the idea of merging CSS-anim and SVG-anim if it doesn't get too confusing, but I'm okay with svg-anim too. Your call!

@Trombach
Copy link
Contributor Author

Hey Martin, thanks so much for taking a look!

I've basically had the same concerns and ideas as you brought up here, which is good. I personally feel like this belongs to the anim expression "namespace" and I only introduced svg-anim for this PoC. In fact, when I first tried to persist my SVG animations, I just naively tried anim and noticed it didn't work, which led me to look into how this feature works under the hood.

Maybe there's a more general improvement to the anim expression that we should consider here. Right now, the code doesn't allow the usage of an expression kind without a key, but it wouldn't be too hard to support keyless expressions. I would propose that the anim expression can be declared without a key which would indicate "all animations, including SVG ones". This would allow for the following scenarios to be supported:

  • data-vtbag-x="anim":
    • iff element is SVGSVGElement store/restore SVG animation state keyed with /svg or another invalid animation name
    • also call getAnimation on element and store/restore all CSS animations
  • data-vtbag-x="anim:/svg"`:
    • iff element is SVGSVGElement store/restore SVG animation
  • data-vtbag-x="anim:my-transition":
    • only store/restore CSS animation with name my-transition
  • data-vtbag-x="anim:/svg anim:my-transition":
    • iff element is SVGSVGElement store/restore SVG animation
    • also store/restore CSS animation with name my-transition

I have no data to back this up, but I feel like the first case would be sufficient for most developers, which could simplify the dx (no need to specify the name, which also makes it more robust against animation name changes).
This approach has one main downside, which is that usage with the short form might look a bit strange, I.e. data-vtbag-x="~". But maybe that's ok? I'm not sure.
This might be out of scope for this PR, but I also feel introducing a new expression type that we might deprecate and roll into anim in the future is also not a good idea.

Happy to go with whatever approach you feel is most appropriate

@martrapp
Copy link
Contributor

I completely agree with everything you say, Lukas!
Let's implement your last three bullet points in this PR right now. ;-)

I think the first point with the meaning “all” is great.
That could also be anim:* or ~*.

But I would take that out of the current PR for now because
a) all “CSS animations” requires that we can also encode the cases where the keyframes identifier is not unique (does not work so far, but could be interesting)
b) I want to check beforehand whether we can (with reasonable effort) do something with CSS transitions, which are also part of the animations. These may then also have an influence on the naming scheme.

@Trombach
Copy link
Contributor Author

That sounds great, and I love the idea with the wildcard key! Makes everything much more consistent. Also agree we should leave this out for now. I'll get right on changing the implementation so that the last 3 points work as explained.

Oh and I had a quick look at persisting transition state and I came across this: https://css-tricks.com/controlling-css-animations-transitions-javascript/ I think a similar approach could be used to save the animation state and then resume it from there on the new page. It's not as ergonomic as for animations and I think you would need to know the name of the property that's being transitioned.

@martrapp
Copy link
Contributor

Thanks for the link! what a lot of information in one article ;-)

martrapp
martrapp previously approved these changes Oct 29, 2024
Copy link
Contributor

@martrapp martrapp left a comment

Choose a reason for hiding this comment

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

Looks good to me!
Thank you very much!

@Trombach
Copy link
Contributor Author

Awesome. I'll update the docs PR as soon as I have a free minute

@martrapp
Copy link
Contributor

Thank you so much, no rush!

@Trombach
Copy link
Contributor Author

@martrapp I've added a changeset with level patch. Feel free to modify it to your liking

@martrapp martrapp merged commit d4fd998 into vtbag:main Oct 30, 2024
1 check passed
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