Skip to content

🚀 [Story performance] Disable animations on first page if story is transformed#37135

Merged
mszylkowski merged 7 commits intoampproject:mainfrom
mszylkowski:disableAnimationsTransformed
Dec 7, 2021
Merged

🚀 [Story performance] Disable animations on first page if story is transformed#37135
mszylkowski merged 7 commits intoampproject:mainfrom
mszylkowski:disableAnimationsTransformed

Conversation

@mszylkowski
Copy link
Copy Markdown
Contributor

@mszylkowski mszylkowski commented Dec 7, 2021

We want to disable the animations on the first page if the story is transformed, to prepare for the CSS split that would glitch (show + hide + animate in) animated elements on the first page.

@mszylkowski mszylkowski requested a review from gmajoulet December 7, 2021 18:04
@mszylkowski mszylkowski self-assigned this Dec 7, 2021
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Dec 7, 2021 •
edited
Loading

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

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

gmajoulet
gmajoulet approved these changes Dec 7, 2021
View reviewed changes
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.

Plz also update this:

amphtml/extensions/amp-story/1.0/amp-story.js

Lines 417 to 425 in 0c200f5

if (
isExperimentOn(this.win, 'story-disable-animations-first-page') ||
isPreviewMode(this.win) ||
prefersReducedMotion(this.win)
) {
Services.performanceFor(this.win).addEnabledExperiment(
'story-disable-animations-first-page'
);
}

@mszylkowski mszylkowski enabled auto-merge (squash) December 7, 2021 18:24
@mszylkowski mszylkowski disabled auto-merge December 7, 2021 18:26
@mszylkowski mszylkowski merged commit 788b14d into ampproject:main Dec 7, 2021
@swissspidy swissspidy mentioned this pull request Dec 14, 2021
Closed
@dreamofabear
Copy link
Copy Markdown

dreamofabear commented Dec 14, 2021

We received a bug report about this recently. Is there any additional context? E.g. is this a permanent change?

@mszylkowski
Copy link
Copy Markdown
Contributor Author

mszylkowski commented Dec 14, 2021

I've mentioned on Slack recently:

...we have temporarily turned on the experiment that disables animations on the first page for now in order to check the performance improvements on all stories with animations as we test other improvements in parallel (...) until the beginning of January (if all goes well, no promises) but we might eventually think of a smarter / more long term approach with better UX.

Hope that gives some context, but I'm happy to also answer any more questions on Slack or here.

@dreamofabear
Copy link
Copy Markdown

dreamofabear commented Dec 14, 2021

Cool, thanks.

Is there some public live documentation we can point non-technical users to re: the current state of experiments? E.g. even a markdown file in amphtml would be nice.

@mszylkowski
Copy link
Copy Markdown
Contributor Author

mszylkowski commented Dec 14, 2021

Yes, there's a JSON file here with the fast-rollout experiments, and here for prod (slow-rollout) experiments

@dreamofabear
Copy link
Copy Markdown

dreamofabear commented Dec 14, 2021

Not great for non-technical users but maybe this is a special case that won't happen too often.

@mszylkowski
Copy link
Copy Markdown
Contributor Author

mszylkowski commented Dec 14, 2021

Could be potentially easy to make a dashboard that parses these JSONs and shows them in a table, not sure if it's worth it for the couple of people that might be interested in them

@ampproject/wg-infra

@swissspidy
Copy link
Copy Markdown
Contributor

swissspidy commented Dec 15, 2021

Did someone say dashboard?

I took your suggestion as a challenge to whip up something very simple:

https://amp-experiments-dashboard.vercel.app/

Source code: https://github.com/swissspidy/amp-experiments-dashboard

@mszylkowski mszylkowski mentioned this pull request Jan 4, 2022
Open
@BrittanyIRL BrittanyIRL mentioned this pull request Apr 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erwinmombay erwinmombay erwinmombay approved these changes

@gmajoulet gmajoulet gmajoulet approved these changes

Assignees

@mszylkowski mszylkowski

Labels

PR use: In Beta / Experimental PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mszylkowski @dreamofabear @swissspidy @erwinmombay @gmajoulet @ampprojectbot