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.

Sign up for GitHub

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

Jump to bottom

🐛Fix zucks ad return timing #22719

Merged
merged 4 commits into from Jun 7, 2019
Merged

🐛Fix zucks ad return timing #22719

merged 4 commits into from Jun 7, 2019

Conversation

yang-wei
Copy link
Contributor

@yang-wei yang-wei commented Jun 6, 2019

Instructions:

@yang-wei yang-wei changed the title :bug Fix zucks ad return timing 🐛 Fix zucks ad return timing Jun 6, 2019
@yang-wei yang-wei changed the title 🐛 Fix zucks ad return timing 🐛Fix zucks ad return timing Jun 6, 2019
@yang-wei
Copy link
Contributor Author

yang-wei commented Jun 6, 2019

@lannka @torch2424 Can you please look at this ? 🙏

torch2424
torch2424 approved these changes Jun 6, 2019
View reviewed changes
Copy link
Contributor

@torch2424 torch2424 left a comment •
edited

Choose a reason for hiding this comment

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

LGTM 😄 Also, thanks for continuing to use your voyagegroup fork, helps us verify code ownership! 👍

@torch2424
Copy link
Contributor

@yang-wei

Before merging this, I went ahead and tested just to make sure things are working, and I got the following:

Screen Shot 2019-06-06 at 4 42 19 PM

Seems like there is an error for the larger ad. If this is expected I'll go ahead and merge.

Thanks! 😄

@torch2424
Copy link
Contributor

@yang-wei

Oh also, seems like the travis check for linting failed. Please run the linter as well and then I can merge this 😄

Thanks!

@yang-wei
Copy link
Contributor Author

yang-wei commented Jun 7, 2019

@torch2424 hey I linted my code. Thanks for checking the ad though. Its fine to not show up for larger ad. (Although I fixed it now)

p/s: you have a very nice profile icon

@torch2424
Copy link
Contributor

@yang-wei

hey I linted my code. Thanks for checking the ad though. Its fine to not show up for larger ad. (Although I fixed it now)

Awesome thanks! 😄

p/s: you have a very nice profile icon

Thank you! 👍 I appreciate it! 😄

Merging this, thank you for the contribution! 👏 🎉

@torch2424 torch2424 merged commit 65ef920 into ampproject:master Jun 7, 2019
@yang-wei yang-wei deleted the fix-zucks-ad-return-timing branch July 10, 2019 00:03
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
@yang-wei @torch2424
🐛Fix zucks ad return timing (ampproject#22719)
9fb483f 
* fix no return. use loadScript

* use writeScript for tag with writeDocument

* less example

* lint ads/zucks.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torch2424 torch2424 torch2424 approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yang-wei @torch2424 @googlebot