Skip to content

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

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

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

Conversation

@mszylkowski

@mszylkowski mszylkowski commented Nov 1, 2021

Copy link
Copy Markdown
Contributor

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

amp-owners-bot Bot commented Nov 1, 2021

Copy link
Copy Markdown

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

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

@gmajoulet gmajoulet left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread extensions/amp-story/1.0/amp-story.js Outdated
@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