Skip to content

🚀 [Story performance] Use dvh if available, instead of vh#37003

Merged
mszylkowski merged 5 commits intoampproject:mainfrom
mszylkowski:dvhVariable
Nov 24, 2021
Merged

🚀 [Story performance] Use dvh if available, instead of vh#37003
mszylkowski merged 5 commits intoampproject:mainfrom
mszylkowski:dvhVariable

Conversation

@mszylkowski
Copy link
Copy Markdown
Contributor

We want to introduce a CSS variable that accurately reflects the viewport height, --story-dvh, that will allow us to inject a script inlined into the document to set the variable when the document loads, so on mobile browsers the pages won't have any CLS

@mszylkowski mszylkowski self-assigned this Nov 18, 2021
@amp-owners-bot amp-owners-bot bot requested a review from estherkim November 18, 2021 21:42
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Nov 18, 2021

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

extensions/amp-story/1.0/amp-story-access.css
extensions/amp-story/1.0/amp-story-desktop-one-panel.css
extensions/amp-story/1.0/amp-story-page.js
extensions/amp-story/1.0/amp-story-share-menu.css
extensions/amp-story/1.0/amp-story.css
extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/pagination-buttons.css
extensions/amp-story/1.0/test/test-amp-story.js

@gmajoulet gmajoulet requested review from erwinmombay and jridgewell and removed request for estherkim November 18, 2021 21:59

amp-story[standalone]:-webkit-full-screen {
height: var(--story-100dvh) !important;
height: 100vh !important;
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.

Why isn't this using var(--story-100dvh)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Full screen pages don't have any issues with vh since the browser appbar is not showing, it should be always the same 1dvh = 1vh.

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.

5 participants