Skip to content

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

Merged
kristoferbaxter merged 1 commit intomainfrom
alanorozco-patch-11
May 24, 2021
Merged

📖 amp-animation: Clarify prefers-reduced-motion#34442
kristoferbaxter merged 1 commit intomainfrom
alanorozco-patch-11

Conversation

@alanorozco
Copy link
Copy Markdown
Member

No description provided.

@alanorozco alanorozco requested review from dmanek and gmajoulet May 19, 2021 14:34
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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