Skip to content

🐛amp-consent: Fixed URL Bar resizing for CMP Iframe modal#20100

Merged
torch2424 merged 2 commits intoampproject:masterfrom
torch2424:amp-consent-ui-safari-nav-bar
Jan 11, 2019
Merged

🐛amp-consent: Fixed URL Bar resizing for CMP Iframe modal#20100
torch2424 merged 2 commits intoampproject:masterfrom
torch2424:amp-consent-ui-safari-nav-bar

Conversation

@torch2424
Copy link
Copy Markdown
Contributor

closes #19969

This fixes URL Bar Resizing for both Mobile Safari and Mobile Chrome for the amp consent modal. In the sense that, the element is not cut off by any of the url/nav bars, and transitions smoothly along with them.

This also includes some small changes to the example, to make mobile testing easier, and fix the diy-consent buttons on mobile safari.

Note

We should document for the full screen case that the safari nav bar can cover the bottom of their content, thus, we should advise them to leave some padding. Our only other option (I could find), was the make the height: 100%, but this will not transition smoothly, and gives a bad UX in my opinion.

GIfs

Before

consentmobilebarsbefore

After

consentmobilebarsafter


/**
* CMP Modal class appolied when asking for CMP consent.
* See: https://developers.google.com/web/updates/2016/12/url-bar-resizing
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @torch2424, the comment is very useful!

I have one question after reading the doc. I saw

There is one exception to the above changes and that is for elements that are position: fixed. Their behavior remains unchanged. That is, a position: fixed element whose containing block is the ICB will resize in response to the URL bar showing or hiding.

Do we still need to change 100% to 100vh if we apply position: fixed here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is because even though the viewport height and percentages are the same end value, during the transition, viewport units will be calculated, but percentage based values will jump from the before and after.

Try the demo they linked in the article: https://bokand.github.io/demo/urlbarsize.html

And you will see that when the url bar hides, the percentage based vertical div will jump sizes, instead of being consistent 😄

* For the reasoning of mixing percentages with viewport units
*/
amp-consent.i-amphtml-consent-ui-iframe-active {
position: fixed !important;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This isn't needed, will should be removed.

Copy link
Copy Markdown
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

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

Thank you @torch2424 for the investigation!! Definitely LGTM!! As we agreed it would be great to move comment around so that people don't change the 100% 100vh here.

* For the reasoning of mixing percentages with viewport units
*/
amp-consent.i-amphtml-consent-ui-iframe-active {
position: fixed !important;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Discussed offline. This should be safe to remove because we've already applied position: fixed to amp-consent.

@torch2424 torch2424 merged commit b5c185c into ampproject:master Jan 11, 2019
@torch2424 torch2424 deleted the amp-consent-ui-safari-nav-bar branch January 11, 2019 01:40
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
…#20100)

* Fixed URL Bar resizing for CMP Iframe modal

* Made CSS Comment changes
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.

AMP-CONSENT v2 UI Followup Design Changes

4 participants