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

Test amp-ad works seamlessly when embedded inside of a PWA #9679

Closed
jasti opened this issue Jun 1, 2017 · 7 comments
Closed

Test amp-ad works seamlessly when embedded inside of a PWA #9679

jasti opened this issue Jun 1, 2017 · 7 comments

Comments

@jasti
Copy link
Contributor

jasti commented Jun 1, 2017

Main thing to ensure is that the ads are loaded and viewability pings (active view) are fired for both AMP and non-AMP ads when they are delivered into an AMP page that is embedded inside of a shadow DOM within a PWA.

@jasti jasti added this to the Sprint H1 June milestone Jun 1, 2017
@jasti jasti added this to Sprint Candidate in AMP Advertising Jun 1, 2017
@lannka
Copy link
Contributor

lannka commented Jun 5, 2017

@choumx do we have an amp-ad example in your PWA shell?

@lannka
Copy link
Contributor

lannka commented Jun 12, 2017

Confirmed with @choumx , 3P ads works: https://choumx.github.io/amp-pwa/content/21.amp.html
Will need to add an example for A4A. @choumx will take a look and see if he has bandwidth for this sprint.

@dreamofabear
Copy link

Tested that an AMP and non-AMP ad render correctly without errors on our local PWA app. Try it out by going to http://localhost:8000/pwa?exp=a4a:-1&force_a4a_ctypes=1,2 and clicking on the "A4A" article.

@jasti @lannka Interestingly, I noticed that the AMP ad doesn't always render faster than the non-AMP ad in PWA with 2G network throttling on Chrome. Instead, the ad with the higher viewport position is rendered first. Might be due to shadow DOM streaming.

@jasti jasti reopened this Jun 30, 2017
@jasti
Copy link
Contributor Author

jasti commented Jun 30, 2017

It's possible we are falling back to using delayed fetch in this case which is not ideal.
@lannka to confirm. Heads-up to @keithwrightbos to consider this case when making FF work for canonical.

@dreamofabear
Copy link

Reassigning to @lannka per comment above. Let me know if I can help.

@jasti jasti modified the milestones: Sprint H2 July, Fixit Week EOQ2 Jul 14, 2017
@jasti jasti moved this from Current Sprint to Sprint Candidate in AMP Advertising Jul 14, 2017
@lannka lannka moved this from Sprint Candidate to Current Sprint in AMP Advertising Jul 28, 2017
@lannka lannka added this to the Sprint H1 August milestone Aug 7, 2017
@lannka lannka removed this from the Sprint H2 July milestone Aug 7, 2017
@jasti jasti moved this from Current Sprint to Sprint Candidate in AMP Advertising Sep 22, 2017
@jasti jasti moved this from Sprint Candidate to Feature Backlog in AMP Advertising Sep 22, 2017
@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @lannka Do you have any updates?

@lannka lannka removed this from Feature Backlog in AMP Advertising Jul 2, 2019
@lannka lannka added this to Needs triage in Ads & Analytics issue triaging via automation Jul 2, 2019
@lannka lannka moved this from Needs triage to Fixit in Ads & Analytics issue triaging Jul 2, 2019
@lannka lannka added this to To do in Ads fixit - August 2019 via automation Aug 5, 2019
@lannka lannka assigned powerivq and unassigned lannka Aug 5, 2019
@powerivq powerivq moved this from To do to In progress in Ads fixit - August 2019 Aug 5, 2019
@powerivq powerivq moved this from In progress to Reviewer approved in Ads fixit - August 2019 Aug 5, 2019
@powerivq powerivq moved this from Reviewer approved to In progress in Ads fixit - August 2019 Aug 5, 2019
@powerivq
Copy link
Contributor

powerivq commented Aug 6, 2019

@jasti It has been tested, and there are no delay on both types of ads on PWA.

@choumx The layout prioritization has been changed to 1 for ads on PWA and that might explain the sequencing.
#12232

I think it's in a fine state now. Feel free to reopen if there's anything concerning.

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

No branches or pull requests

5 participants