♻️ Require trigger="visibility" to run an <amp-story-animation> effect

[alanorozco]

This disambiguates effect specs that trigger with page entrance versus those that are triggered by other specs (subanimations).

Note that subanimations are not currently supported, but this change prevents issues when introducing the feature.

[amp-owners-bot]

Hey @newmuis, @Enriqe! These files were changed:

extensions/amp-story/1.0/animation.js
extensions/amp-story/1.0/test/test-animation.js

[gmajoulet]

Can you elaborate on your plans for the trigger parameter on amp-story-animation? (This trigger="in" is not documented)
What's the API going to look like for subanimations? No trigger at all?

[dvoytenko]

Can you elaborate on your plans for the trigger parameter on amp-story-animation? (This trigger="in" is not documented)
What's the API going to look like for subanimations? No trigger at all?

Correct. Subanimations have no trigger at all. TBH, I don't like everything about it, but we thought that adding a trigger for now would be safer - it will keep the door open for sub-animations done the old way. And if we find a good new way, we can always default it and make it optional.

The "in" is a new value, just for stories. We can instead go to "visible", which is a standard value.

[gmajoulet]

Sounds good thanks for clarifying :))

"visible" might be easier to understand, or if we want to match the analytics events we send on page navigation for consistency, something like "story-page-visible".

[dvoytenko]

Sounds good thanks for clarifying :))

"visible" might be easier to understand, or if we want to match the analytics events we send on page navigation for consistency, something like "story-page-visible".

In that case I'd vote for trigger="visibility". It fits ok with the original intent of amp-animation.

[alanorozco]

Using visibility. PTAL