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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation on 2021-background preset slightly inaccurate for web stories #34011

Closed
alankent opened this issue Apr 26, 2021 · 7 comments · Fixed by #34083
Closed

Documentation on 2021-background preset slightly inaccurate for web stories #34011

alankent opened this issue Apr 26, 2021 · 7 comments · Fixed by #34083

Comments

@alankent
Copy link

What's the issue?

Documentation for https://amp.dev/documentation/components/amp-story-grid-layer/#modern-aspect-ratio says "This preset covers the entire screen ... without letterboxing". That is not correct - it can still letter box. What happens is it resizes to cover the screen, cropping at most 7% (as mentioned). If more than 7% is required then it resorts to letterbox still.

Just need to word the text to say letterboxing is only resorted to if more than 7% would be cropped.

How do we reproduce the issue?

I used the following

  <amp-story-page id="modern">
    <amp-story-grid-layer preset="2021-background" template="fill">
      <amp-img src="001-01-9x16.png" width="720" height="1280" layout="responsive"></amp-img>
    </amp-story-grid-layer>
  </amp-story-page>

Resizing the output gives letterbox at top or sides as appropriate, cropping some of the image. The follow screenshots use red fill to highlight iPad and iPhone X sizes from Chrome Developer Tools to show letterboxing can happen at the top or sides for extreme device sizes.

image

image

What browsers are affected?

All browsers - it is an aspect ratio issue.

Which AMP version is affected?

The 2021-background preset I assume is new in 2021.

@gmajoulet
Copy link
Contributor

cc @mszylkowski can you help?

@mszylkowski
Copy link
Contributor

Yes, will create a PR with improved wording. The doc is confusing but it says "it covers the entire screen on all mobile devices ... without letterboxing", which is right. The iPhone X preset on the Chrome Devtools is not accurate on viewport sizes for the story (it's too tall) but the #development=1 mode should have a better representation of the iPhone X if you check the iPhone 11 Pro, which has the same screen size. Using the #development=1, there shouldn't be any letterboxing with the background preset on mobile devices. On iPads there will be letterboxing.

@alankent
Copy link
Author

Cool. Using "iPad" on Chrome in Dev Tools considers it mobile - iPad Pro it does not consider mobile - but not sure if the same error there. But I used it on a story and it is a nice mid point between the two extremes of zero crop and zero letterbox.

@alankent
Copy link
Author

Hmmm. Sorry, just looking closer, is this expected? DevTools in chrome again. It has cropped at top and sides at the same time. Does that mean it always crops? I was hoping it would only crop if needed.

image

@mszylkowski
Copy link
Contributor

Yes, it will always crop since it's meant to be a background layer. The feature makes it so that either you have a layer that doesn't get cropped at all (background), or a layer that will cover the phone screens but get cropped (background). The main functionality of the preset layers is that you can add any elements on the foreground, and then have a background that will stay perfectly aligned with these foreground elements. Check this image (that should be in the documentation but got lost in between branch name changes):

@alankent
Copy link
Author

So it is more of a "reveal" mode. It reserves 7% off the sides and tops so it can reveal more of the image if on a taller or wider display. So not exactly what I had in mind, but it achieves the same purpose. Basically that 7% should be considered "pretty but not important" so it can always be cropped.

@mszylkowski
Copy link
Contributor

mszylkowski commented Apr 26, 2021

Yes, the difference is that the current preset logic tries to always maximize the size of the safe-area, while what you explain is trying to minimize the background cropping. The reason for using what we have is that the foreground elements are the most important things to show on the page, so maximizing their size is key. For use-cases like yours, usually creating an aspect-ratio layer with the ratio of the image will make it contained on the page (just like object-fit: contain). "Minimizing the background cropping" can be achieved with a regular grid layer and layout="fill".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants