Skip to content

Avoid showing the JS code when GA tracker is not defined#149

Merged
DonSchado merged 1 commit intorailslove:masterfrom
chalofa:patch-2
Dec 17, 2019
Merged

Avoid showing the JS code when GA tracker is not defined#149
DonSchado merged 1 commit intorailslove:masterfrom
chalofa:patch-2

Conversation

@chalofa
Copy link
Copy Markdown
Contributor

@chalofa chalofa commented Dec 9, 2019

No description provided.

@chalofa
Copy link
Copy Markdown
Contributor Author

chalofa commented Dec 9, 2019

with Rails 6, in the system tests, I'm seeing all these Google Analytics extra events as page content:
image

Not exactly sure what Rails change caused this issue, but before Rails v6, the tests rendered those GA events as:

Rails 5.2:

<head>
...
<script type="text/javascript">
  ga("ec:setAction","checkout",{"step":"1"});
  ga("ec:addImpression",...
  ga(...)
</script>
</head>
<body>
...
</body>

(note the lack of the GA snippet, but at least, everything is wrapped in a <script> tag

in Rails 6:

<head>
...
ga("ec:setAction","checkout",{"step":"1"});
ga("ec:addImpression",...
ga(...)
</head>
<body>
...
</body>

there's no <script> that wraps that JS code, and therefore, the browser just shows it as visible text...

the change in this PR ensures that if there's no GA tracker, nothing will be rendered (which is the workaround that I'm using right now to avoid this: ensuring rack-tracker is disabled for the test environment)

@chalofa
Copy link
Copy Markdown
Contributor Author

chalofa commented Dec 9, 2019

@DonSchado looks like you are pretty active with this Google Analytics tracker... if you can take a look at this PR, I would appreciate it...

probably way easier to see without all those whitespace changes:
https://github.com/railslove/rack-tracker/pull/149/files?w=1

@DonSchado
Copy link
Copy Markdown
Collaborator

DonSchado commented Dec 16, 2019

I don't get the diff... is this just a change of indentation? Or also a scope change?

@chalofa
Copy link
Copy Markdown
Contributor Author

chalofa commented Dec 16, 2019 •
edited
Loading

I don't get the diff... is this just a change of indentation? Or also a scope change?

@DonSchado basically, wrap the whole <script> tag inside the top if tracker...

@DonSchado
Copy link
Copy Markdown
Collaborator

DonSchado commented Dec 17, 2019

hm wasn't that already fixed here: f3d4e65 ?
Sorry I'm confused

@chalofa
Copy link
Copy Markdown
Contributor Author

chalofa commented Dec 17, 2019

hm wasn't that already fixed here: f3d4e65 ?
Sorry I'm confused

@DonSchado nope, the indentation is totally messed up, see this image:

image

I added some indentation so it's easier to see the nested ifs, and move the whole template inside the if tracker conditional, so if no tracker is defined, it won't render those events that are outside that conditional

@DonSchado
Copy link
Copy Markdown
Collaborator

DonSchado commented Dec 17, 2019

Ah I see. Thanks for taking the time!
Very appreciated :)

@DonSchado DonSchado merged commit 2ab8a68 into railslove:master Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chalofa @DonSchado