add support for piano analytics#2075
add support for piano analytics#2075nathangoulding wants to merge 4 commits intoampproject:masterfrom
Conversation
|
Moved to #2075 |
|
@cramforce was this closed by mistake? I can't find the correct PR to review. |
|
Re-opening as I believe this was closed by mistake. |
| } | ||
| </script> | ||
| </amp-analytics> | ||
| <!-- End Piano example --> |
There was a problem hiding this comment.
There doesn't seem to be a start tag for this. Want to add
<!-- Start Piano example -->
before the example?
|
PTAL |
| 'requests': { | ||
| 'host': 'https://api-v3.tinypass.com', | ||
| 'basePrefix': '/api/v3', | ||
| 'baseSuffix': '&pageview_id=${pageViewId}&rand=${random}', |
There was a problem hiding this comment.
This looks unused. Did you mean to have it on the pageview request below?
There was a problem hiding this comment.
Ah yes, thanks for catching this.
|
A couple more small things. @avimehta PTAL as well |
| 'timezone_offset=${timezone}&tags=${tags}&_url=${ampdocUrl}&' + | ||
| 'screen=${screenWidth}x${screenHeight}', | ||
| }, | ||
| 'transport': { |
There was a problem hiding this comment.
The defaults will take care of this if all the values are true. Feel free to remove this section.
There was a problem hiding this comment.
OK will do. As per the previous code review, after I remove the transport block, should I keep the trailing comma at the end of the requests?
|
lgtm. (assuming the comments above are addressed.) |
|
I'll rebase again and open another PR. |
Tracked in issue: #1769