-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
Added initial version of amp-anlaytics variable filters. #6560
Conversation
e4badf3
to
51e9f0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, assuming this is the same code minus the hasOwn
changes?
@@ -46,6 +46,8 @@ variableServiceFor(AMP.win); | |||
|
|||
const MAX_REPLACES = 16; // The maximum number of entries in a extraUrlParamsReplaceMap | |||
|
|||
const hasOwn = Object.prototype.hasOwnProperty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be useful as a utility in /utils/object.js
?
Yes. Following up on this. Given the large scale changes in this code and the fact that I am going to be ooo next few weeks, I'll submit this in Jan. I'll try and send out a PR to create a function in object.js tomorrow but the other filter changes will go in in Jan. |
👍 |
Branched from ampproject#6560
* Added hasOwn helper. Branched from #6560 * Review feedback.
* Added hasOwn helper. Branched from ampproject#6560 * Review feedback.
* Added hasOwn helper. Branched from ampproject#6560 * Review feedback.
d5340e2
to
1f7f437
Compare
1f7f437
to
61ee6ca
Compare
@jridgewell PTAL. |
s.push(`${encodeURIComponent(k)}=${sv}`); | ||
} | ||
} | ||
|
||
const paramString = s.join('&'); | ||
const paramString = s.length > 0 ? s.join('&') : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ternary is unnecessary.
Closes #5621.