Improve Digital AMP integration #2678
Conversation
|
Thanks, @Re-lM! Could you please run |
|
|
||
| const url = 'https://ad.360yield.com' + | ||
| '/adj?' + | ||
| 'p=' + data.placement + |
There was a problem hiding this comment.
Since all data properties arrive from outside, we have to encode them via encodeURIComponent.
|
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. |
|
@dvoytenko I updated the files. Can you have a look? Thanks. |
|
CLAs look good, thanks! |
| '&tz=' + new Date().getTimezoneOffset(); | ||
|
|
||
| if (typeof data.keyvalue !== 'undefined' && data.keyvalue) { | ||
| url += '&' + encodeURIComponent(data.keyvalue); |
There was a problem hiding this comment.
Is this really what you want? If data.keyvalue is, for instance, "a=b", they will be encoded in your code as &a%3Db. If it works for your URL, it's certainly fine, but I wanted to make sure.
|
Looks good. I posted one question about |
|
@dvoytenko actually I rather not encode the data.keyvalue. Can i? |
| @@ -0,0 +1,42 @@ | |||
| /** | |||
| * Copyright 2015 The AMP HTML Authors. All Rights Reserved. | |||
There was a problem hiding this comment.
2016 here and every other new file
|
@Re-lM Unfortunately - no encoding is not an option in this case. But encoding this should be rather straightforward. You just need to split |
Re-lM
force-pushed
the
master
branch
2 times, most recently
from
3765c65
to
cff290d
Compare
|
@dvoytenko done. Squashed the commits. Oke for merge? Let me know if I need to change something. Thanks. |
|
|
||
| if (value && value.length > 0) { | ||
| const keys = value.split('&'); | ||
| for (let i = 0;i < keys.length;i++) |
There was a problem hiding this comment.
nit: spaces between components of for, e.g. for (let i = 0; i < keys.length; i++)
|
@Re-lM Yes, that's right. Just couple more notes and clarifications and please squash again after. Otherwise - it looks all good to be merged. Thanks! |
Re-lM
force-pushed
the
master
branch
3 times, most recently
from
694f397
to
529f5f9
Compare
|
@dvoytenko Adjusted it. Can you review? Squashed the commit. |
| if (!keys[i]) {continue;} | ||
| const segment = keys[i].split('='); | ||
| const segment1 = (segment[1] ? encodeURIComponent(segment[1]) : ''); | ||
| if (keys[i]) { |
There was a problem hiding this comment.
Why are you checking keys[i] again here if you already check it above in if (!keys[i]) {continue;}? Did you mean to test if (segment[0]) instead?
There was a problem hiding this comment.
@dvoytenko I'm doing another check to see if the second and third or any possibility could be empty. Like keys ["", "", "", "gender=male", "placement=999", "", "", "", "plavement=1"]. Resulting to not have a double && in the URL.
There was a problem hiding this comment.
@Re-lM I meant that you have these two statements:
if (!keys[i]) {continue;}
and
if (keys[i]) {....}
My point was that with the first if the second is always true.
There was a problem hiding this comment.
@dvoytenko aah see what you mean. You're right. Just tested it and removed it. re-squashed.
fix squase fix conflict Improve Digital AMP integration fix lint issues update year update year Update keyvalue logic Fix variable Fix variable fixing spaces removing unnecessary code
|
LGTM |
|
@dvoytenko Thanks! |
Initial integration for Improve Digital in AMP.