Skip to content

🖍 [story desktop panel] make aspect ratio match 2021-background for short viewports#36275

Merged
kristoferbaxter merged 3 commits intoampproject:mainfrom
cpauwels:fix-35796
Oct 12, 2021
Merged

🖍 [story desktop panel] make aspect ratio match 2021-background for short viewports#36275
kristoferbaxter merged 3 commits intoampproject:mainfrom
cpauwels:fix-35796

Conversation

@cpauwels
Copy link
Contributor

@cpauwels cpauwels commented Oct 7, 2021

Closes #35796 and avoids situations like this one:

image

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2021

CLA assistant check
All committers have signed the CLA.

@amp-owners-bot amp-owners-bot bot requested a review from Enriqe October 7, 2021 11:17
@amp-owners-bot
Copy link

amp-owners-bot bot commented Oct 7, 2021

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

extensions/amp-story/1.0/amp-story-desktop-one-panel.css

@mszylkowski
Copy link
Contributor

Thanks for contributing, I'm adding some context so other people in the team can review this:

Context: As I suggested on the issue #35796, the maximum aspect-ratio 36/53 would match the widest phone experiences for stories properly. Currently the aspect-ratio for the one-panel can go up to 3/4, but that's too wide / square for the ratios that we see in the wild, bringing more headaches to story creators. This PR aims to limit the aspect-ratio of the one-panel to that of the wider set of phone screens, so creators don't need to account for even wider formats than what they have to.

I think this PR is good to go, and we could focus on really tall displays at some point with #35285 (which is not as prevalent and would require some extra work).

Any other thoughts? @gmajoulet @newmuis @processprocess

@processprocess
Copy link
Contributor

Thank you for the PR and additional context!
I think it makes sense for this to follow the max aspect ratio we communicate.

Do we want to consider adding 3 / 4 as the widest aspect ratio we support?
iPads are 3 / 4 (There may not be a large amount of people viewing stories on iPads so not sure if it's worth it).

It would be good to follow up with @hongcatlover to understand this decision before we modify this UX.

@cpauwels
Copy link
Contributor Author

cpauwels commented Oct 7, 2021

@processprocess I dont't own an iPad but I think (according to the Chrome device preview) it already switches to the desktop mode, and hence uses the default 69 / 116 aspect ratio.

Here's a screenshot:
CleanShot 2021-10-07 at 17 56 02@2x

@gmajoulet
Copy link
Contributor

Quick questions:

  • Why did we override the default 69:116 with 3:4 for short heights in the first place?
  • Why don't we use 69:116 here too?

@cpauwels
Copy link
Contributor Author

cpauwels commented Oct 7, 2021

I think the idea was that in landscape on phones the story would be as large as possible. But IMO 3 / 4 is just too wide, and I think the 36 / 53 aspect ratio works just right, while also being consistent with the 2021-background.

@mszylkowski
Copy link
Contributor

Quick questions:

  • Why did we override the default 69:116 with 3:4 for short heights in the first place?
  • Why don't we use 69:116 here too?

I think we could use 69:116 on one-panel desktop at all times and remove the height breakpoint at 538px, I don't have a preference. I also see the point on having a wider page size when the browser window is short, so we can check with other people in the wg.

Just an important note on this change: if we change anything here, we also need to change css/amp-story-player-shadow.css.

@cpauwels
Copy link
Contributor Author

cpauwels commented Oct 7, 2021

I've updated the PR with the changes needed on amp-story-player-shadow.css

@gmajoulet
Copy link
Contributor

Deferring to @processprocess on what aspect ratio to use, but the change sounds good to me.

@cpauwels can you add a // If changing this, also change amp-story-player-shadow.css type comments on both variables? Thank you!

@cpauwels
Copy link
Contributor Author

cpauwels commented Oct 8, 2021

@gmajoulet comments added to the two changed files mentioning the linked file

Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

Looks great to me.
Thank you for following up with this and writing the PR Paul!

@gmajoulet gmajoulet enabled auto-merge (squash) October 8, 2021 18:51
@processprocess
Copy link
Contributor

@newmuis we need you're OWNERS blessing on this :)

@kristoferbaxter kristoferbaxter merged commit 67edb06 into ampproject:main Oct 12, 2021
@processprocess
Copy link
Contributor

Thank you for your contribution @cpauwels!
It's great how this ties the desktop experience into the aspect ratio work.
it's awesome to have your perspective and skill on this. 💪

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.

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

8 participants