Skip to content

🐛 [amp story page attachment] min-height for drawers when in supports-landscape mode#37277

Merged
processprocess merged 3 commits intoampproject:mainfrom
processprocess:supports-landscape-drawer
Jan 18, 2022
Merged

🐛 [amp story page attachment] min-height for drawers when in supports-landscape mode#37277
processprocess merged 3 commits intoampproject:mainfrom
processprocess:supports-landscape-drawer

Conversation

@processprocess
Copy link
Copy Markdown
Contributor

@processprocess processprocess commented Dec 28, 2021

Attachments in supports-landscape mode appear squished on small screens.
Screen Shot 2021-12-28 at 11 58 08 AM

This PR sets a min height on desktop mode attachments.
The desktop attribute is currently only set when in DESKTOP_FULLBLEED.
For a separate PR, this attribute should be renamed to desktop-fullbleed since it's specific to that UI type.

Implementation screen recording.

This PR removes a calc in the drawer height that appears to be dead code.

Context / Fixes #37234

@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Dec 28, 2021

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

extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.css

@mszylkowski
Copy link
Copy Markdown
Contributor

mszylkowski commented Dec 28, 2021

One nit we should verify before merging: when the page attachment content is very short, does the modal expand it's height to the whole page or does it max-out at the height of the content? (I think the latter is the ideal behavior)

mszylkowski
mszylkowski approved these changes Dec 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!

@jaygray0919
Copy link
Copy Markdown

jaygray0919 commented Jan 18, 2022

Is a solution here still on track?
Rather than using our instances, we check one of our model sites:
https://www.nationalgeographic.com/amp-stories/travel/best-trips-2020/?sf227538199=1
My recollection is that the attachment did not need overflow-y

Similarly, this target did not need overflow-y; on desktop landscape the attachment was 100% viewable:
https://www.washingtonpost.com/graphics/2019/lifestyle/travel/amp-stories/a-locals-guide-to-what-to-eat-and-do-in-new-york-city/
/j

@processprocess
Copy link
Copy Markdown
Contributor Author

processprocess commented Jan 18, 2022

Thank you for the ping @jaygray0919
This fix is ready, it will be merged shortly.

@processprocess processprocess merged commit 3212ffe into ampproject:main Jan 18, 2022
@processprocess processprocess deleted the supports-landscape-drawer branch January 18, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mszylkowski mszylkowski mszylkowski approved these changes

Assignees

No one assigned

Labels

PR use: In Beta / Experimental PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reduced area of amp-story-attachment

4 participants

@processprocess @mszylkowski @jaygray0919 @ampprojectbot