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
Add a vendor filter to analytics-vendors.amp.html for easier testing. #9389
Add a vendor filter to analytics-vendors.amp.html for easier testing. #9389
Conversation
build-system/app.js
Outdated
var ads = file.match(new RegExp('<(amp-ad|amp-embed) [^>]*[\'"]' | ||
+ req.query.type + '[\'"][^>]*>([\\s\\S]+?)<\/(amp-ad|amp-embed)>', 'gm')); | ||
file = file.replace( | ||
/<body>[\s\S]+<\/body>/m, '<body>' + ads.join('') + '</body>'); | ||
} | ||
|
||
// Extract amp-ad for the given 'type' specified in URL query. |
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.
update comment.
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.
done
1. "optout": if needed. We currently don't have a great opt-out system, so please reach out to help us design one that works well for you. | ||
1. An example in the [examples/analytics-vendors.amp.html](../../examples/analytics-vendors.amp.html) | ||
1. A line for your analytics service in the 'type' attribute section of the [amp-analytics](amp-analytics.md) usage reference. | ||
1. Test the new example you put in [examples/analytics-vendors.amp.html](../../examples/analytics-vendors.amp.html) to ensure the hits from the example are working as expected. For example, the data needed is being collected and displayed in your analytics dashboard. |
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.
optional: say something about type parameter for the example file?
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 dropdown UI is intuitive, isn't it? :-)
build-system/app.js
Outdated
var ads = file.match(new RegExp('<(amp-ad|amp-embed) [^>]*[\'"]' | ||
+ req.query.type + '[\'"][^>]*>([\\s\\S]+?)<\/(amp-ad|amp-embed)>', 'gm')); | ||
file = file.replace( | ||
/<body>[\s\S]+<\/body>/m, '<body>' + ads.join('') + '</body>'); | ||
} | ||
|
||
// Extract amp-ad for the given 'type' specified in URL query. | ||
if (req.path.indexOf('/examples/analytics-vendors.amp.html') == 0 && req.query.type) { | ||
var analytics = file.match(new RegExp('<amp-analytics [^>]*[\'"]' |
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.
optional: move the creation of regex out to a function?
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.
done
<form action="analytics-vendors.amp.html" method="get" target="_top"> | ||
<label for="filter">Vendor filter</label> | ||
<select id="filter" name="type"> | ||
<option>[Select a vendor]</option> |
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.
It is not clear exactly why one needs to select a vendor. Maybe put it somewhere?
"Select a vendor to show config for that vendor." or something along these lines. I initially thought that selecting a type will allow you to run only that analytics type.
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.
your initial thought is actually right
@avimehta PTAL |
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.
Thanks for helping with this cleanup!
No description provided.