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

📖 amp-animation: Clarify prefers-reduced-motion #34442

Merged
merged 1 commit into from
May 24, 2021

Conversation

alanorozco
Copy link
Member

No description provided.

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Documenting publicly what we said privately:

This is too advanced for publishers to know about, think about, and too complex to write. We need to automatically solve this at the amp-story-animation level for publishers.

@alanorozco
Copy link
Member Author

alanorozco commented May 19, 2021

@gmajoulet From our private chat as well.

I fundamentally disagree with the approach being "too complicated", since the author includes the pre-animation CSS intentionally in any case. It's also exactly the way you have to do it on any document, it doesn't depend on our constraints nor the AMP format.

What you're asking is not feasible at this level. Anything else would make the animations engine render-blocking, which is against our performance concerns.

If you'd like to solely update the implementation for <amp-story-animation> then by all means, but you're going to get a terrible user experience unless you make it render-blocking. If you make it render-blocking, the whole document will take longer to paint.

@gmajoulet
Copy link
Contributor

Two things:

  1. A document that takes longer to paint is better than a document that does not paint
  2. We're a team of solid and experienced frontend engineers, we will together find a solution that works for our users and delivers great UX and performance

@ampproject ampproject locked as resolved and limited conversation to collaborators May 24, 2021
@kristoferbaxter
Copy link
Contributor

Going to merge this PR as is and schedule some time for a followup conversation to discuss the points brought up.

Thank you all for working to make this happen and looking forward to the next steps.

@kristoferbaxter kristoferbaxter merged commit 739890d into main May 24, 2021
@gmajoulet
Copy link
Contributor

Matias is working on a proper fix here: #34466

We know the ecosystem well enough to know that adding a few lines in the documentation and expect publishers to write complex code to solve something they don't even know of won't work. I don't think we need to meet to discuss this further.

@rsimha rsimha deleted the alanorozco-patch-11 branch August 27, 2021 16:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants