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

ALP for A4A #5430

Merged
merged 7 commits into from
Oct 7, 2016
Merged

ALP for A4A #5430

merged 7 commits into from
Oct 7, 2016

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Oct 5, 2016

Work for #5212

  • Tested with a fake A4A. I failed to get a working A4A ad that navigates to a valid amp page. So I tested that we can get the redirect url correctly from an A4A ad. Then I mocked a fake url in our code base and tested that we get the desired redirect behavior. I think these two tests get the redirect process fully covered, we should be good now with an experiment flag.
  • Another thing @cramforce mentioned, and I think we need to finalize is what to do with target = _blank in the case of A2A redirect. This is a preference thing, I would vote for open in new window since user specifically ask for target=_blank.

cc @jasti

@zhouyx zhouyx changed the title [WIP] ALP for A4A ALP for A4A Oct 6, 2016
@zhouyx
Copy link
Contributor Author

zhouyx commented Oct 6, 2016

PTAL

Copy link
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

LGTM with one comment

if (!isExperimentOn(this.win, 'alp-for-a4a')) {
return;
}
iframeWin.addEventListener('click', event => {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? I think iframeWin.document.documentElement would be more idiomatic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha! changed to register listener on element.

Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

Great work. LGTM

@lannka lannka added LGTM and removed NEEDS REVIEW labels Oct 7, 2016
@zhouyx zhouyx merged commit 744f158 into ampproject:master Oct 7, 2016
@zhouyx zhouyx deleted the alp branch October 7, 2016 18:13
samiamorwas pushed a commit to samiamorwas/amphtml that referenced this pull request Oct 10, 2016
…mp_reddit_extension

* 'master' of https://github.com/ampproject/amphtml: (65 commits)
  Send vars from carousel to amp-analytics (ampproject#5388)
  Rolling back pan-x on scrollable carousel. (ampproject#5490)
  Add context.library.name to Segment (ampproject#5467)
  Rollback pan-x on carousel as it does not work on android. (ampproject#5473)
  cron job from @erwinmombay to update size.txt (ampproject#5468)
  Added example of usage of carousel arrows (ampproject#5397)
  ALP for A4A (ampproject#5430)
  Add Affiliate-B support for amp-ad (ampproject#5223)
  First cut of changes for New Type Inference (ampproject#5438)
  Lightbox 2.0: Use object-fit:cover for thumbnails
  Upgrade Closure Compiler to v20160911 (Sep 11, 2016) (ampproject#5457)
  Fake and real window fixtures for tests (ampproject#5425)
  Backgrounded Variable added to Timer Trigger (ampproject#5240)
  make `touch-action: pan-x` on `amp-carousel` (ampproject#5391)
  Fix PR check on new branches (ampproject#5455)
  Do not rely on ampExtendedElements being created (ampproject#5454)
  Turn type checking for 3p code back on. (ampproject#5452)
  Update ads/README.md with gulp lint tip (ampproject#5444)
  Migrate document-submit to doc scope (ampproject#5437)
  Link to Building an AMP Extension document. (ampproject#5451)
  ...
dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Oct 12, 2016
mkhatib pushed a commit to mkhatib/amphtml that referenced this pull request Oct 14, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
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.

None yet

3 participants