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

[desktop one page] 2021-background preset not optimally aligning #35796

Closed
cpauwels opened this issue Aug 25, 2021 · 6 comments · Fixed by #36275
Closed

[desktop one page] 2021-background preset not optimally aligning #35796

cpauwels opened this issue Aug 25, 2021 · 6 comments · Fixed by #36275

Comments

@cpauwels
Copy link
Contributor

Description

The 2021-background and 2021-foreground amp-story-grid-layer presets are not working great with the recently launched desktop one page visualisation.

localhost_3000_public_amp-template html (6)

Here's an example where the story has 3 different backgrounds: the desktop blurred background, the gray amp-story-page background and the amp-story-grid-layer preset background. Would it make sense to limit the aspect-ratio of the story to what is possible to display with the 2021-background preset, or alternatively make the amp-story-page background transparent so it reveals the blurred image?

Alternatives Considered

No real alternatives possible other than setting a background on the amp-story-page.

Additional Context

No response

@cpauwels cpauwels changed the title [desktop one page] 2021-background / 2021-foreground presets not optimally aligning [desktop one page] 2021-background preset not optimally aligning Aug 25, 2021
@newmuis
Copy link
Contributor

newmuis commented Aug 25, 2021

I like both of the proposals here! My vote would be to just use the specified aspect ratio as the aspect ratio of the page.

/cc @processprocess @mszylkowski @hongcatlover

@mszylkowski
Copy link
Contributor

mszylkowski commented Aug 25, 2021

Thanks for bringing this up, I also agree that the aspect ratio of the page should never be wider than the widest phone we're handling currently (as it just brings more variance into the design, and the blurred background works great to cover that extra bit of space). Only when the screen is very short (in height) the aspect-ratio is set to 3/4, but regularly it's set to 69/116 which is correct (here are the CSS rules for that).

We can either keep it always at 69/116 or change the aspect ratio to the widest configuration supported of around 36/53 or 67.9%.

@newmuis
Copy link
Contributor

newmuis commented Aug 25, 2021

Sorry, I interpreted the initial request here to mean we should use the actual publisher-specified aspect ratio, when it is provided, falling back to the format-specified default only if the publisher-specified aspect ratio is unspecified (or, I guess, maybe if it's crazy like 1000:1)

@mszylkowski
Copy link
Contributor

the initial request here to mean we should use the actual publisher-specified aspect ratio

This could be weird if different pages have different ratios (you could do that to support different types of layouts, though it's no ideal), and introduce even more edge cases for story creators. If we wanted more weirdly shaped stories (eg: 1:1) I'd probably suggest to create an API that allows pubs to specify their preferred aspect-ratio for the whole story, which might not be that beneficial anyways. Fixing the aspect ratio of the page to work better with the new presets (and stay within the phone sizes we've seen) sounds more in line to fix the screenshot attached IMO.

Also, @cpauwels what was the feature request more concretely? Was it to fix the borders when using the new presets or was it an issue in general with any aspect-ratio templates?

@cpauwels
Copy link
Contributor Author

cpauwels commented Aug 26, 2021

The issue is about fixing the borders when using the 2021 presets. Here are a few sketches I was made:

Frame 1 (4)

Frame 2 (1)

I guess the best way to proceed would be to limit the widest and tallest a story can be (dark red frame in the pictures) to the limits of the 2021-background preset (that's 36/56 for the width and 25/48 for the height?)

I'm also guessing this impacts #35285 which should then not be 1/2 but 25/48 as the tallest aspect ratio?

@cpauwels
Copy link
Contributor Author

cpauwels commented Oct 7, 2021

@processprocess @mszylkowski I created a PR (#36275) to close this issue. We really want to switch to using the 2021 background and foreground but this issue is blocking us, as it looks terrible with the borders around the background layer. Currently we override the CSS in the custom styles but this obviously makes the stories not amp valid anymore. Can you check if everything is OK for a merge? (sorry this is the first PR I'm making in this repo)

@processprocess processprocess added this to To do in wg-stories Desktop One Panel via automation Oct 12, 2021
@processprocess processprocess added this to To do in wg-stories Sprint via automation Oct 12, 2021
@processprocess processprocess moved this from To do to Done in wg-stories Sprint Oct 12, 2021
@processprocess processprocess moved this from To do to Done in wg-stories Desktop One Panel Oct 12, 2021
danielrozenberg pushed a commit to danielrozenberg/amphtml that referenced this issue Oct 12, 2021
… short viewports (ampproject#36275)

* Makes story-desktop-one-panel aspect ratio match `2021-background` aspect ratio (fixes ampproject#35796)

* Updates player aspect ratio on short viewport height

* Adds comment mentioning the linked change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants