Skip to content
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.

Sign up for GitHub

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

Fix AMP validation when tracking is disabled #1252

Merged
merged 12 commits into from
Mar 16, 2020
Merged

Fix AMP validation when tracking is disabled #1252

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
95a2cc1
Don't print amp-analytics if tracking disabled.
aaemnnosttv Mar 13, 2020
74f5d47
Don't output tracking opt-out in AMP context.
aaemnnosttv Mar 13, 2020
34a945a
Update no-js selector to exclude valid amp contexts.
aaemnnosttv Mar 13, 2020
8c26d3a
Add optoutElementId to amp analytics config.
aaemnnosttv Mar 16, 2020
20d520b
Update tracking opt out for AMP compatibility.
aaemnnosttv Mar 16, 2020
f2af477
Remove guard to rely on opt-out in AMP again.
aaemnnosttv Mar 16, 2020
a89657a
Replace meta as opt-out element with plain script.
aaemnnosttv Mar 16, 2020
bb9ed73
Simplify printing tracking opt out.
aaemnnosttv Mar 16, 2020
edded94
Use application/ld+json as type for now.
aaemnnosttv Mar 16, 2020
8328b65
Remove irrelevant todo comment.
aaemnnosttv Mar 16, 2020
17e7019
Set optoutElementId at top level.
aaemnnosttv Mar 16, 2020
8e30592
Add support for optoutElementId to AMP Tag Manager snippet.
felixarntz Mar 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion assets/sass/components/global/_googlesitekit-noscript.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
}
}

.no-js {
.no-js:not([amp-version]) {

[id^="js-googlesitekit-"] {
display: none;
Expand Down
9 changes: 7 additions & 2 deletions includes/Modules/Analytics.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,12 +384,13 @@ protected function print_amp_gtag() {
'vars' => array(
'gtag_id' => $tracking_id,
'config' => array(
$tracking_id => array(
$tracking_id => array(
'groups' => 'default',
'linker' => array(
'domains' => array( $this->get_home_domain() ),
),
),
'optoutElementId' => '__gaOptOutExtension',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to be on the top-level, together with vars. See e.g. https://github.com/ampproject/amphtml/blob/b0aff1b9a59d3ca8f10a6ebe4d6eaeda8c826e61/extensions/amp-analytics/0.1/vendors/googleanalytics.json#L65

),
),
);
Expand Down Expand Up @@ -1273,7 +1274,11 @@ private function get_home_domain() {
private function print_tracking_opt_out() {
?>
<!-- <?php esc_html_e( 'Google Analytics user opt-out added via Site Kit by Google', 'google-site-kit' ); ?> -->
<script type="text/javascript">window["_gaUserPrefs"] = { ioo : function() { return true; } }</script>
<?php if ( $this->context->is_amp() ) : // TODO: update script type to `text/plain` when supported by AMP plugin. ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the comment here per ampproject/amp-wp#4392

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I suppose that is no longer relevant 😄

Done!

<script type="application/ld+json" id="__gaOptOutExtension"></script>
<?php else : ?>
<script type="text/javascript">window["_gaUserPrefs"] = { ioo : function() { return true; } }</script>
<?php endif; ?>
<?php
}

Expand Down