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

馃悰 Added back JS for vh #36719

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Nov 2, 2021

Fixes #36711

Reverts removing the JS units calculation from #36033

Before / After (URLs match the original story and the demo story with change)

Before After

Also tested, and the styles for the desktop one-panel are not affected by the bug, and are also not affected by this fix (those styles are overriden). Also, the desktop one-panel is not shown on mobile UIs.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Nov 2, 2021

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

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

@mszylkowski mszylkowski self-assigned this Nov 2, 2021
Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but a test would be a good idea.

@mszylkowski
Copy link
Contributor Author

Given how this behavior relies on certain browsers' 1vh being different from 1% I can model a visual test that replicates this behavior but it would take some time to get right (bc visual tests run really slow and they aren't meant to test these things). I can add the visual test on a followup.

@mszylkowski mszylkowski merged commit 4b43359 into ampproject:main Nov 2, 2021
wg-stories Sprint automation moved this from In progress to Done Nov 2, 2021
ampprojectbot pushed a commit that referenced this pull request Nov 2, 2021
(cherry picked from commit 4b43359)
ampprojectbot pushed a commit that referenced this pull request Nov 2, 2021
(cherry picked from commit 4b43359)
this.cssVariablesStyleEl_.setAttribute('type', 'text/css');
doc.head.appendChild(this.cssVariablesStyleEl_);
}
this.cssVariablesStyleEl_.textContent = `:root {--story-page-vh: ${px(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why isn't this just an inline style on the <html> element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically for this PR it's because it used to be like that, so I reverted that back.

In general, I think inlining the style would have made more sense (+ you don't have to keep track of this extra element).

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] Broken layouts for any publisher using vh units
5 participants