Add fluct to amp-ad support by saxsir · Pull Request #8176 · ampproject/amphtml

saxsir · 2017-03-16T07:00:30Z

Hi, we want to add fluct into the supported SSP for amp-ad.

We have signed Google Corporate CLA.

Corporation name: fluct, Inc.

Thank you very much !

ampprojectbot · 2017-03-16T07:00:32Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko jridgewell

3p/integration.js
extensions/amp-ad/amp-ad.md

/to lannka zhouyx

ads/_config.js
ads/fluct.js
ads/fluct.md

/to camelburrito lannka

examples/ads.amp.html

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

googlebot · 2017-03-16T07:00:32Z

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.

saxsir · 2017-03-21T01:47:30Z

@ampprojectbot retry!

googlebot · 2017-03-21T01:47:32Z

CLAs look good, thanks!

ampprojectbot · 2017-03-21T01:47:33Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko jridgewell

3p/integration.js

/to lannka zhouyx

ads/_config.js
ads/fluct.js
ads/fluct.md

/to camelburrito lannka

examples/ads.amp.html

/to lannka zhouyx

extensions/amp-ad/amp-ad.md

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

saxsir · 2017-03-22T06:54:07Z

@lannka Hi, now CI test has all passed, I would like you to review our code.

saxsir · 2017-04-21T01:14:23Z

@zhouyx Could you review this PR?

zhouyx · 2017-04-21T21:12:37Z

examples/ads.amp.html

+      type="fluct"
+      data-g="1000067784"
+      data-u="1000101409">
+    <div placeholder></div>


with default placeholder supported. no need to add placeholder and fallback here.

zhouyx · 2017-04-21T21:13:24Z

extensions/amp-ad/amp-ad.md

 - [E-Planning](../../ads/eplanning.md)
 - [Ezoic](../../ads/ezoic.md)
 - [FlexOneELEPHANT](../../ads/f1e.md)
+- [fluct](../../ads/fluct.md)


Fluct please

Our corporate name is "fluct"(not "Fluct").

https://corp.fluct.jp/en/

zhouyx · 2017-04-21T21:15:16Z

ads/fluct.md

+
+For more information, please [contact us](https://corp.fluct.jp/en/contact.php).
+
+Supported parameters:


From your code, data-g and data- should be mandatory. please indicate they're required in the doc.

zhouyx · 2017-04-21T21:15:48Z

ads/fluct.js

+ */
+export function fluct(global, data) {
+  validateData(data, ['g', 'u']);
+  writeScript(global, `https://cdn-fluct.sh.adingo.jp/f.js?G=${data['g']}`, function() {


use encodeURIComponent to wrap data['g'] please

please follow the max 80 characters rule.

zhouyx · 2017-04-21T21:18:32Z

ads/_config.js

+      'https://cdn-fluct.sh.adingo.jp',
+      'https://s.sh.adingo.jp',
+      'https://i.adingo.jp',
+    ],


Please consider implementing the render-start and no-content-available APIs (see Available APIs), which helps AMP to provide user a much better ad loading experience.

Please let me know if there's any question

We are not supported API which kind of detect ad status(start rendring and no content) yet.

ampproject#8176 (comment)

ampproject#8176 (comment) ampproject#8176 (comment)

saxsir · 2017-04-27T05:00:27Z

@zhouyx

Thank you for your review.
We fixed some points you commented, please have a look.

saxsir · 2017-04-27T09:59:42Z

@ampprojectbot retry!

zhouyx

@saxsir LGTM. Just one minor request

zhouyx · 2017-04-28T22:52:16Z

ads/fluct.js

+export function fluct(global, data) {
+  validateData(data, ['g', 'u']);
+  writeScript(global,
+    `https://cdn-fluct.sh.adingo.jp/f.js?G=${encodeURIComponent(data['g'])}`,


extra indentation needed for code can't fall in one line. (need 2 more here and below).

@zhouyx Thx, added indent here.

saxsir · 2017-05-01T02:33:25Z

Fixed indent.

zhouyx · 2017-05-01T17:25:33Z

@saxsir LGTM. Please rebase to sync with master in order to make travis green.

Remove placeholder and fallback ampproject#8176 (comment) Add required param description ampproject#8176 (comment) Use encodeURIComponent ampproject#8176 (comment) ampproject#8176 (comment) indent

saxsir · 2017-05-02T03:02:27Z

rebased.

@zhouyx Thank you very much!

zhouyx · 2017-05-04T22:08:42Z

Oops. @saxsir seems someone changes amp-ad.md as well. Please rebase again to fix the conflict, and then it should be good to be merged.

Remove placeholder and fallback ampproject#8176 (comment) Add required param description ampproject#8176 (comment) Use encodeURIComponent ampproject#8176 (comment) ampproject#8176 (comment) indent

saxsir · 2017-05-05T09:08:38Z

Oh... rebased. @zhouyx

zhouyx · 2017-05-05T16:55:27Z

@saxsir Thanks for the Pull Request. Merged.

saxsir force-pushed the fluct-amp-ad branch from 22cd078 to f5336dd Compare March 16, 2017 07:13

jridgewell requested a review from zhouyx April 21, 2017 02:31

jridgewell added the WG: monetization label Apr 21, 2017

zhouyx suggested changes Apr 21, 2017

View reviewed changes

saxsir pushed a commit to voyagegroup/amphtml that referenced this pull request Apr 27, 2017

Remove placeholder and fallback

61d7b5d

ampproject#8176 (comment)

saxsir pushed a commit to voyagegroup/amphtml that referenced this pull request Apr 27, 2017

Add required param description

0a1e494

ampproject#8176 (comment)

saxsir pushed a commit to voyagegroup/amphtml that referenced this pull request Apr 27, 2017

Use encodeURIComponent

c2edc8e

ampproject#8176 (comment) ampproject#8176 (comment)

zhouyx suggested changes Apr 28, 2017

View reviewed changes

zhouyx approved these changes May 1, 2017

View reviewed changes

zhouyx self-assigned this May 1, 2017

saxsir force-pushed the fluct-amp-ad branch from d883020 to a45fc99 Compare May 2, 2017 01:46

Add fluct SSP support for amp-ad

6fd9912

Remove placeholder and fallback ampproject#8176 (comment) Add required param description ampproject#8176 (comment) Use encodeURIComponent ampproject#8176 (comment) ampproject#8176 (comment) indent

saxsir force-pushed the fluct-amp-ad branch from a45fc99 to 6fd9912 Compare May 5, 2017 09:08

googlebot added the cla: yes label May 5, 2017

zhouyx merged commit 88cc483 into ampproject:master May 5, 2017

tetsuharuohzeki deleted the fluct-amp-ad branch May 11, 2017 04:32

ampprojectbot added the PR use: In Beta / Experimental label May 11, 2017


		For more information, please [contact us](https://corp.fluct.jp/en/contact.php).

		Supported parameters:

Conversation

saxsir commented Mar 16, 2017

Uh oh!

ampprojectbot commented Mar 16, 2017

Uh oh!

googlebot commented Mar 16, 2017

Uh oh!

saxsir commented Mar 21, 2017

Uh oh!

googlebot commented Mar 21, 2017

Uh oh!

ampprojectbot commented Mar 21, 2017

Uh oh!

saxsir commented Mar 22, 2017

Uh oh!

saxsir commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saxsir commented Apr 27, 2017

Uh oh!

saxsir commented Apr 27, 2017

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saxsir commented May 1, 2017

Uh oh!

zhouyx commented May 1, 2017

Uh oh!

saxsir commented May 2, 2017

Uh oh!

zhouyx commented May 4, 2017

Uh oh!

saxsir commented May 5, 2017

Uh oh!

zhouyx commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zhouyx Apr 21, 2017 •

edited

Loading