Skip to content

🚀 [Story performance] Rewrite styles only on desktop one-panel or bot rendering#36692

Merged
mszylkowski merged 9 commits intoampproject:mainfrom
mszylkowski:runStyleReplacementOnDektopOnly
Nov 3, 2021
Merged

🚀 [Story performance] Rewrite styles only on desktop one-panel or bot rendering#36692
mszylkowski merged 9 commits intoampproject:mainfrom
mszylkowski:runStyleReplacementOnDektopOnly

Conversation

@mszylkowski
Copy link
Copy Markdown
Contributor

@mszylkowski mszylkowski commented Nov 1, 2021

Closes #36660

The vw/vh/vmin/vmax values in mobile match the story-page-vw/vh/vmin/vmax values on mobile, so we don't need to replace them (on mobile or full-bleed desktop). We will replace then only on one-panel and on vertical-rendering (because the height of the page is not 100vh).

Note that the regex function now runs about 30-40% faster (/(-?[\d.]+)v(w|h|min|max)/gim) due to the condensed group matching instead of the chained replaces, and makes the JS bundle a bit smaller.

@mszylkowski mszylkowski requested a review from gmajoulet November 1, 2021 15:53
@mszylkowski mszylkowski self-assigned this Nov 1, 2021
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Nov 1, 2021

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

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

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.

can you revert the package-lock changes before submitting?

@mszylkowski mszylkowski merged commit 53a2742 into ampproject:main Nov 3, 2021
@mszylkowski mszylkowski deleted the runStyleReplacementOnDektopOnly branch November 3, 2021 20:13
rileyajones pushed a commit to rileyajones/amphtml that referenced this pull request Nov 4, 2021
… rendering (ampproject#36692)

* rewrite styles only on desktop

* Updated package-lock

* Change naming of variable

* Reverted package lock

* Reverted package lock for real

* Added newline to package json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Story] Only run amp-story.rewriteStyles_() when needed

3 participants