Skip to content

Relap support for amp-ads#7011

Merged
lannka merged 5 commits intoampproject:masterfrom
advzr:relap
Jan 30, 2017
Merged

Relap support for amp-ads#7011
lannka merged 5 commits intoampproject:masterfrom
advzr:relap

Conversation

@advzr
Copy link
Copy Markdown
Contributor

@advzr advzr commented Jan 12, 2017 •
edited by jridgewell
Loading

Closes #7010

@advzr
Add ads/relap
d24929d 
Partial commit

Update Relap
@googlebot
Copy link
Copy Markdown

googlebot commented Jan 12, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@advzr
Copy link
Copy Markdown
Contributor Author

advzr commented Jan 12, 2017

I signed it!

@googlebot
Copy link
Copy Markdown

googlebot commented Jan 12, 2017

CLAs look good, thanks!

@aghassemi aghassemi requested a review from lannka January 12, 2017 16:33
@aghassemi
Copy link
Copy Markdown
Contributor

aghassemi commented Jan 12, 2017

/to @lannka for review

@advzr
Copy link
Copy Markdown
Contributor Author

advzr commented Jan 20, 2017

Hi. I don't want to hurry you, but when can I expect your review? My employer is asking me about it every day.

@jridgewell
Copy link
Copy Markdown
Contributor

jridgewell commented Jan 20, 2017

Ping @lannka, @zhouyx

@jridgewell jridgewell requested a review from zhouyx January 20, 2017 20:36
@lannka
Copy link
Copy Markdown
Contributor

lannka commented Jan 25, 2017

@advzr would you try to follow the guide and implement the render-start API?

@lannka lannka removed the request for review from zhouyx January 25, 2017 00:20
@advzr
Copy link
Copy Markdown
Contributor Author

advzr commented Jan 30, 2017

@lannka Done

lannka
lannka approved these changes Jan 30, 2017
View reviewed changes
Copy link
Copy Markdown
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.

Looks great, some minor comments.

examples/ads.amp.html Outdated
</amp-ad>

<h2 id="relap">Relap</h2>
<amp-ad width=auto height=750
Copy link
Copy Markdown
Contributor

@lannka lannka Jan 30, 2017

Choose a reason for hiding this comment

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

nitpick: double quote around attribute values

examples/ads.amp.html Outdated
data-token="D3UMgQWBqleq1tPW"
data-url="http://bigpicture.ru"
data-anchorid="i0xMMY1MoliiZWVl"
layout="fixed-height"
Copy link
Copy Markdown
Contributor

@lannka lannka Jan 30, 2017

Choose a reason for hiding this comment

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

nit: could you move layout before all the data attributes?

examples/ads.amp.html
data-url="http://bigpicture.ru"
data-anchorid="i0xMMY1MoliiZWVl"
layout="fixed-height"
>
Copy link
Copy Markdown
Contributor

@lannka lannka Jan 30, 2017

Choose a reason for hiding this comment

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

move this to previous line

examples/ads.amp.html Outdated
</amp-ad>

<h2 id="relap">Relap</h2>
<amp-ad width=auto height=750
Copy link
Copy Markdown
Contributor

@lannka lannka Jan 30, 2017

Choose a reason for hiding this comment

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

Is relap about content recommendation or ads? For content recommendation, amp-embed tag is recommended, see taboola

Copy link
Copy Markdown
Contributor Author

@advzr advzr Jan 30, 2017

Choose a reason for hiding this comment

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

relap is about content recommendation with ads. The amount of ads depends on the publisher. The widget can be without any ads, have some or consist of only ads.

@lannka lannka merged commit 5eb7fa9 into ampproject:master Jan 30, 2017
@advzr advzr deleted the relap branch January 31, 2017 05:45
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Feb 14, 2017
@advzr @torch2424
Relap support for amp-ads (ampproject#7011)
280e89c 
* Add ads/relap

Partial commit

Update Relap

* Fix to pass CI build

* Add renderStart support

* Change attributes order

* Add double quotes around attribute values
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
@advzr @mrjoro
Relap support for amp-ads (ampproject#7011)
62ab8fa 
* Add ads/relap

Partial commit

Update Relap

* Fix to pass CI build

* Add renderStart support

* Change attributes order

* Add double quotes around attribute values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lannka lannka lannka approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lannka lannka

Labels

WG: monetization

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@advzr @googlebot @aghassemi @jridgewell @lannka @zhouyx