Skip to content

amp-consent consent-ui: Show/Hide the AMP Viewer when amp-consent CMPs enterFullScreen#23491

Merged
torch2424 merged 5 commits intoampproject:masterfrom
torch2424:consent-viewer-overlay
Jul 27, 2019
Merged

amp-consent consent-ui: Show/Hide the AMP Viewer when amp-consent CMPs enterFullScreen#23491
torch2424 merged 5 commits intoampproject:masterfrom
torch2424:consent-viewer-overlay

Conversation

@torch2424
Copy link
Copy Markdown
Contributor

@torch2424 torch2424 commented Jul 23, 2019

relates to #22943 (Maybe closes, but still investigating the issue)
closes #22994

This hides/shows the viewer bar (similar to amp-lightbox), when the consent-ui goes into the fullscreen mode 😄

Example

amp consent example hide viewer on full screen

@torch2424 torch2424 requested a review from zhouyx July 23, 2019 21:28

this.resetAnimationStyles_();

this.viewer_.sendMessage(
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.

I think we should use the Viewport.enterLightboxMode() instead?

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.

So enterLightboxMode() does a lot more than I think is neccessary. see my response here to @jridgewell 😄

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.

I do see that it handles fixed layer and and FIE.

In our case, the FIE won't be an issue because the request can never be from a component in FIE. But I find it's doesn't hurt to include it.
Not sure about fixed layer. @jridgewell could you please help us understand if the message is enough here instead of calling Viewport.enterLightBoxMode(). Thanks.

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.

If it's an already fixed position element, it might not need any special fixed-layer support?

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.

Yeah I don't think it requires any special fixed-layer support 😄

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.

Sounds good. Thanks!


this.resetAnimationStyles_();

this.viewer_.sendMessage(
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.

If it's an already fixed position element, it might not need any special fixed-layer support?

@zhouyx
Copy link
Copy Markdown
Contributor

zhouyx commented Jul 27, 2019

Thank you @torch2424 for figuring this out! 🎉

@torch2424
Copy link
Copy Markdown
Contributor Author

Thank you @zhouyx and @jridgewell ! Merging...

@torch2424 torch2424 merged commit e705649 into ampproject:master Jul 27, 2019
@torch2424 torch2424 deleted the consent-viewer-overlay branch July 27, 2019 00:44
gmajoulet added a commit to gmajoulet/amphtml that referenced this pull request Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

amp-consent CMP: AMP CDN url bar overlays fullscreen Prompt UI iframe

4 participants