Conversation
|
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! |
|
/to @lannka |
|
I signed it! |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
I signed it! |
lannka
left a comment
There was a problem hiding this comment.
Would you also add an example in ads.amp.html and make sure it loads ad locally.
ads/mywidget.js
Outdated
|
|
||
| // load the myWidget initializer asynchronously | ||
| loadScript(global, 'https://likemore-go.imgsmail.ru/widget.amp.js', () => {}, () => { | ||
| global.context.noContentAvailable(); |
There was a problem hiding this comment.
you can make global.context.noContentAvailable as the last param of loadScript?
ads/mywidget.js
Outdated
|
|
||
| ready: function(opts) { | ||
| // Make sure ready() is called only once. | ||
| if (isReady) { return; } |
There was a problem hiding this comment.
please break lines before } and after {
ads/mywidget.js
Outdated
| ready: function(opts) { | ||
| // Make sure ready() is called only once. | ||
| if (isReady) { return; } | ||
| else { isReady = true; } |
ads/mywidget.js
Outdated
| renderStart: global.context.renderStart, | ||
| noContentAvailable: global.context.noContentAvailable, | ||
|
|
||
| ready: function(opts) { |
There was a problem hiding this comment.
this can be simplified to:
ready(opts) {
ads/mywidget.js
Outdated
| const height = data.height; | ||
| let isReady = false; | ||
|
|
||
| if (!(height >= 0)) { |
There was a problem hiding this comment.
height < 0 would not quite fit :) data.height can be undefined here, if attribute has invalid value or is absent, so... I can change condition to: if (typeof height === 'undefined' || height < 0) if you like. But there is no guarantee, that the default value of absent attribute not will be null tomorrow :)
There was a problem hiding this comment.
:-) please add a little comment about this trap. to prevent "smart" people like me to refactor it later.
lannka
left a comment
There was a problem hiding this comment.
could you follow the CLA robot's guide? try to use the same email for your git
ads/mywidget.js
Outdated
| const height = data.height; | ||
| let isReady = false; | ||
|
|
||
| if (!(height >= 0)) { |
There was a problem hiding this comment.
:-) please add a little comment about this trap. to prevent "smart" people like me to refactor it later.
|
I signed it! |
|
I recently synchronized our fork with current But I don't understand what should I do with CLA stuff :) I already commited with my github accout one time f3b83b8b and after that created an individual CLA and said "I signed it!". I missed something? |
Can you try this: 1) set git email to the one signed CLA. 2) squash all the commits and then update this PR. |
|
I think, squash is not possible, because I pulled master between commits, so I have to squash other commits too in that case. What if we just close this pull request and create another with correct git user? Is that ok? |
|
Done! There is new clean pull-request: #7471 |
Pull-request for "myWidget" ad.
Closes #6965