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.

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 gatsby-plugin-google-gtag #9032

Merged
merged 4 commits into from
Oct 16, 2018
Merged

Conversation

tylerthebuildor
Copy link
Contributor

This PR adds support for Google's Global Site Tag (gtag) and closes #8341 you can view that issue for a detailed breakdown.

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Looking great! Left a number of comments, thanks for the PR!

To use it, simply import it and use it like you would the `<a>` element e.g.

```jsx
import React
Copy link
Contributor

Choose a reason for hiding this comment

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

import React from 'react'

@@ -0,0 +1,116 @@
# gatsby-plugin-google-gtag

Easily add Google Global Site Tag to your Gatsby site.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorta tangential to this PR, but could be helpful to document/link to why here, e.g. why this over gatsby-plugin-google-analytics

}
if(${
typeof pluginOptions.respectDNT !== `undefined` &&
pluginOptions.respectDNT == true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use strict equality here? ===

: setPostBodyComponents

const renderHtml = () => {
return `
Copy link
Contributor

Choose a reason for hiding this comment

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

Merely a suggestion, but when I have a few conditions like this, I like to use an array, e.g.

const condOne = true;
const condTwo = false;
const condThree = true;

const html = []
  .concat(condOne && `condition one is true`)
  .concat(condTwo && `condition two is true`)
  .concat(condThree && `condition three is true`)
  .filter(condition => typeof condition === `string`)
  .join("\n");

something to consider!


const excludeGtagPaths = []
if (typeof pluginOptions.pluginConfig.exclude !== `undefined`) {
const Minimatch = require(`minimatch`).Minimatch
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe hoist this to the top for clarity? (even if we lose the benefit of requiring only when it's needed)

import { Minimatch } from 'minimatch'


If you want to call a custom event you have access to `window.gtag` where you can call an event for all products:

```bash
Copy link
Contributor

Choose a reason for hiding this comment

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

js | javascript

If you want to call a custom event you have access to `window.gtag` where you can call an event for all products:

```bash
window.gtag('event', 'click', { ...data });
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to reference that this needs to be guarded against for SSR, e.g.

typeof window !== 'undefined' && window.gtag('event', 'click', {...data})

anonymize_ip: true,
},
// This object is used for configuration specific to this plugin
pluginConfig: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pieh do we have a standard for this? i.e. plugin specific options that are separate from the service you're configuring?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK there is no convention for that, it's totally up to plugins.

In this instance I'm not sure if there is value in grouping those settings in pluginConfig object - whole options is basically pluginConfig, but I don't mind having it this way as long as it's documented.


## `<OutboundLink>` component

To make it easy to track clicks on outbound links the plugin provides a component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's worth calling this ExternalLink, instead of OutboundLink? Is Outbound a term used by gtag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, but it is already a gtag term https://support.google.com/analytics/answer/7478520?hl=en

@@ -0,0 +1,73 @@
import React from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

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

Few unit tests on this would be great!

"url": "https://github.com/gatsbyjs/gatsby/issues"
},
"dependencies": {
"@babel/runtime": "^7.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

need to add deps used here: minimatch is at least one of those

@tylerthebuildor
Copy link
Contributor Author

Thanks, @DSchau and @pieh for the feedback I really appreciate it. I'll work on fixing up this PR after work today 😄

build/Release

# Dependency directory
# https://www.npmjs.org/doc/misc/npm-faq.html#should-i-check-my-node_modules-folder-into-git
Copy link
Contributor

Choose a reason for hiding this comment

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

What about removing all the comments and sort the directories..?

Copy link
Contributor

Choose a reason for hiding this comment

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

This comes from package template - https://github.com/gatsbyjs/gatsby/blob/master/plop-templates/package/.npmignore.hbs it doesn't hurt to have comments here as this is mostly setup once and forget

@tylerthebuildor
Copy link
Contributor Author

I think all the outstanding things have been addressed. I'm not sure what I have to do to make the 4 failing checks pass. All the local tests are passing for me.

@DSchau
Copy link
Contributor

DSchau commented Oct 15, 2018

@tylerbuchea I think the CI tests are failing because this branch isn't up to date with (specifically) the latest CI scripts/config. If you merge upstream into this branch, the tests should pass.


# 1.0.0-beta.0 (2018-06-17)

**Note:** Version bump only for package gatsby-plugin-google-gtag
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did that changelog came from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about that, I copied it from one of the other plugins. Should I just update the date?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tylerbuchea let's just go ahead and remove it if that's OK! lerna should handle the generation for us when we're ready to release!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got you, yep sounds good 👍

@DSchau
Copy link
Contributor

DSchau commented Oct 16, 2018

@tylerbuchea looks good to me. All good to merge?

@tylerthebuildor
Copy link
Contributor Author

Yessir 🎉

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

👍 Thanks for the PR; we appreciate it!

@DSchau DSchau merged commit eadb0f3 into gatsbyjs:master Oct 16, 2018
@gatsbot
Copy link

gatsbot bot commented Oct 16, 2018

Holy buckets, @tylerbuchea — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@extesy
Copy link

extesy commented Oct 17, 2018

I'm trying to use this plugin and get this build error:

- The plugin "gatsby-plugin-google-gtag@1.0.0" is exporting a variable named "__esModule" which isn't an API.
error
Your plugins must export known APIs from their gatsby-ssr.js.
The following exports aren't APIs. Perhaps you made a typo or your plugin is outdated?

Does anyone else see this? This is the only plugin that produces this error and it started after I replaced the previous google-analytics plugin with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Google Global Site Tag Plugin
5 participants