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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

mszylkowski
Copy link
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 self-assigned this Nov 1, 2021
@amp-owners-bot
Copy link

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
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?

extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
@mszylkowski mszylkowski added this to In progress in wg-stories Sprint via automation Nov 3, 2021
@mszylkowski mszylkowski merged commit 53a2742 into ampproject:main Nov 3, 2021
wg-stories Sprint automation moved this from In progress to Done 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
Development

Successfully merging this pull request may close these issues.

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