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

Create a working prototype for AMP lightbox in ads #7743

Closed
4 of 7 tasks
jasti opened this issue Feb 23, 2017 · 5 comments
Closed
4 of 7 tasks

Create a working prototype for AMP lightbox in ads #7743

jasti opened this issue Feb 23, 2017 · 5 comments

Comments

@jasti
Copy link
Contributor

jasti commented Feb 23, 2017

  • The lightbox type expansion should only be allowed if the creative itself is a signed AMP ad (creative in AMP format).
  • The lightbox should go into full screen mode on supported browsers
  • Lightbox expansion should work in scenarios where the parent document is iframed in a viewer
  • What goes into the full screen mode could be a full AMP document or could be partial AMP elements like
  • Define a schema that allows carousel type swipe loading multiple AMP documents or multiple partial AMP elements
  • Consider prefetching/ prerendering lightbox elements when AMP creative comes into current viewport
    Related to Whitelist amp-lightbox as part of an AMP ad #7015
  • Create a container amp-ad-banner tag that will define the content to be repositioned when resizing the iframe into lightbox mode.

CC @dvoytenko @lannka

@jasti
Copy link
Contributor Author

jasti commented Feb 23, 2017

Also requested in #2736

@dvoytenko dvoytenko changed the title Create a working prototype for AMP lightbox ads Create a working prototype for AMP lightbox in ads Feb 24, 2017
@alanorozco alanorozco self-assigned this Feb 26, 2017
@jasti jasti modified the milestones: Sprint H1 March, Pending Triage Feb 27, 2017
@alanorozco
Copy link
Member

alanorozco commented Mar 1, 2017

The lightbox should go into full screen mode on supported browsers

This needs further discussion. Personal opinion is that it would be bad UX.

Lightbox expansion should work in scenarios where the parent document is iframed in a viewer

Proof of concept done for AMP host + A4A. Non-AMP host + A4A case needs further thought.

  • What goes into the full screen mode could be a full AMP document or could be partial AMP elements
  • Define a schema that allows carousel type swipe loading multiple AMP documents or multiple partial AMP elements

I think this is part of a larger story concerning nested AMP documents rather than the lightbox element itself. @lannka, wdyt?

Consider prefetching/prerendering lightbox elements when AMP creative comes into current viewport

This is a good possible optimization. Two things:

  • Prefetching could make a big impact as part of the nested documents story.
  • I'm not sure prerendering would make as big a dent, as it would most likely take less than one animation frame to render the lightbox after user action (and iframe resizing does a vsync pass anyway).

@dvoytenko
Copy link
Contributor

The lightbox should go into full screen mode on supported browsers
This needs further discussion. Personal opinion is that it would be bad UX.

It should NOT go to full screen mode. But rather to the "lightbox" mode (see Viewport.enterLightboxMode). The one change we need to try out and implement is to resize ad iframe to fit the screen to accommodate this feature.

Proof of concept done for AMP host + A4A. Non-AMP host + A4A case needs further thought.

For Non-AMP host we need to ask around to see what strategies are used today. There gotta be some protocol to allow this.

I think this is part of a larger story concerning nested AMP documents rather than the lightbox element itself.

@aghassemi had a fuller story on this.

@alanorozco
Copy link
Member

alanorozco commented Mar 13, 2017

Updated.

The lightbox should go into full screen mode on supported browsers

Done for lightbox mode. Refactoring and testing for feature is required before being able to whitelist.

Lightbox expansion should work in scenarios where the parent document is iframed in a viewer

More context required, will update on Monday.

What goes into the full screen mode could be a full AMP document or could be partial AMP elements like

Embedding external documents is out of scope for this feature. Since the feature uses a normal amp-lightbox, a partial AMP document tree is already supported.

Define a schema that allows carousel type swipe loading multiple AMP documents or multiple partial AMP elements

As mentioned, partial AMP documents are already supported. Embedding documents is out of scope.

Consider prefetching/prerendering lightbox elements when AMP creative comes into current viewport

Prefetching is out of scope since this does not support embedding external documents. Prerendering is a premature optimization.

Added: Create a container amp-ad-banner tag that will define the content to be repositioned when resizing the iframe into lightbox mode.

@alanorozco
Copy link
Member

Closing as "working prototype" has been created. Full feature will be figured out later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants