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

AdsLoom AMP Integration #22758

Merged
merged 3 commits into from Jun 19, 2019
Merged

Conversation

safzal0906
Copy link
Contributor

Hi there,
We'd like to integrate our Ad-Server with AMP. We've contributed to AMP as per documentation. Please check & approve.
Thanks in advance!
Regards,
Shahzaib

@safzal0906
Copy link
Contributor Author

Any update on this?

@safzal0906
Copy link
Contributor Author

/cc @zhouyx
@zhouyx could you please review my code?

Copy link
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.

LGTM. Could you please fix the lint check error so that we can merge the PR.
You could try gulp lint --local-changes --fix locally

@safzal0906
Copy link
Contributor Author

LGTM. Could you please fix the lint check error so that we can merge the PR.
You could try gulp lint --local-changes --fix locally

lint check error fixed

@safzal0906
Copy link
Contributor Author

Hi @zhouyx ,
Can you please merge this PR now, or anything else required on my end?
Thanks!

@torch2424
Copy link
Contributor

torch2424 commented Jun 18, 2019

@safzal0906 @zhouyx

Went ahead and re-ran the flaking saucelabs test 😄

I'll take a look at this tomorrow if not already travis approved and merged. Thanks! 👍

@safzal0906
Copy link
Contributor Author

@torch2424 Thank you for you response. Seems like I don't have required privileges to run individual saucelabs tests. I can only trigger all tests, by doing another commit. Should I go for it, Or is there something else that I can do?

@torch2424
Copy link
Contributor

@safzal0906

Actually, yes, could you pull in the latest master? I re-ran the test for you 3 times. But it seems to keep failing on the same analytics case (which your code does not touch 🤔 )

cc @zhouyx if you see this while travis is still failing, can you take a look?

@zhouyx
Copy link
Contributor

zhouyx commented Jun 18, 2019

I don't have a good answer on why the travis keeps failing. Can we try pull in the latest master

@safzal0906
Copy link
Contributor Author

@torch2424 @zhouyx , Let me pull latest master and try again

@safzal0906
Copy link
Contributor Author

Hurrah! I've pulled latest master and pushed it. All checks have passed now :) @torch2424 @zhouyx guys please merge it now

@torch2424
Copy link
Contributor

Awesome! I also did a quick test, and everything looks good on our end:

Screen Shot 2019-06-19 at 1 30 17 PM

Thank you very much for the contribution @safzal0906 ! 😄 🎉

@torch2424 torch2424 merged commit 8077654 into ampproject:master Jun 19, 2019
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* AdsLoom AMP Integration

* Fixed LGTM issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
New Ads Vendor PR
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants