Skip to content

🚀 [Story performance] Move aspect-ratio logic to CSS for styling#36061

Merged
mszylkowski merged 6 commits intoampproject:mainfrom
mszylkowski:aspectratio_tocss
Oct 14, 2021
Merged

🚀 [Story performance] Move aspect-ratio logic to CSS for styling#36061
mszylkowski merged 6 commits intoampproject:mainfrom
mszylkowski:aspectratio_tocss

Conversation

@mszylkowski
Copy link
Copy Markdown
Contributor

@mszylkowski mszylkowski commented Sep 14, 2021 •
edited
Loading

Closes #36048

Moves the logic to calculate the size of the [aspect-ratio] layer (or [preset]) to the CSS.

The calculations are:

ratio: w/h (eg: 69/116)
unscaled-width: min(page-width, page-height * ratio) = min(page-width, page-height * ratio-width / ratio-height)
unscaled-height: min(page-height, page-width / ratio) = min(page-width, page-height / ratio-height * ratio-width)
width: unscaled-width * scaling-factor
height: unscaled-height * scaling-factor

The CSS properties needed for this to work (which are created by the runtime or in the CSS styles, but should be added with transformers to benefit from early rendering) are: --aspect-ratio: w/h (which is different from the attribute aspect-ratio="w:h")
Eg:

<style>
[aspect-ratio="2:1"] {
    --aspect-ratio: 2/1; /* Layer with aspect-ratio="2:1" should have this CSS variable defined to benefit from early CSS loading */
}
</style>

@mszylkowski mszylkowski self-assigned this Sep 14, 2021
gmajoulet
gmajoulet reviewed Sep 17, 2021
View reviewed changes
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.

Is this ready for review?

@mszylkowski
Copy link
Copy Markdown
Contributor Author

mszylkowski commented Sep 21, 2021

@gmajoulet Tests don't fully pass, but implementation is ready. I'll mark this PR as ready when tests are good.

@mszylkowski mszylkowski marked this pull request as ready for review September 21, 2021 19:25
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Sep 21, 2021

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

extensions/amp-story/1.0/amp-story-grid-layer.js
extensions/amp-story/1.0/amp-story.css
extensions/amp-story/1.0/test/test-amp-story-grid-layer.js

gmajoulet
gmajoulet reviewed Sep 24, 2021
View reviewed changes
extensions/amp-story/1.0/amp-story.css

--i-amphtml-aspect-ratio-calculated: calc(var(--aspect-ratio)) !important; /* Avoid fractions, which don't work with compilers that remove parenthesis */
--i-amphtml-story-page-width: calc(100 * var(--story-page-vw, 1vw)) !important;
--i-amphtml-story-page-height: calc(100 * var(--story-page-vh, 1vh)) !important;
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet Sep 24, 2021

Choose a reason for hiding this comment

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

Plz update these variable names, it's too easy to get into collisions at some point with the one-panel-page-height etc

Copy link
Copy Markdown
Contributor Author

@mszylkowski mszylkowski Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There are no spared variables now, but if they collide, they would have exactly the same value (as it's literally getting the width and height of the page regardless of UI type), and I could just remove the usage from here. I'm open to moving them up to an ancestor of the grid-layers also, but they aren't currently needed anywhere else afaik. Still think it's worth it to rename it to sth like --i-amphtml-story-ancestor-page-height or similar?

@mszylkowski mszylkowski requested a review from gmajoulet October 4, 2021 19:41
@mszylkowski mszylkowski merged commit 6b53dc6 into ampproject:main Oct 14, 2021
@mszylkowski mszylkowski deleted the aspectratio_tocss branch October 14, 2021 14:46
@processprocess processprocess mentioned this pull request Oct 25, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

Assignees

@mszylkowski mszylkowski

Labels

PR use: In LTS PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Story performance] Make aspect-ratio pages work in CSS

3 participants

@mszylkowski @gmajoulet @ampprojectbot