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

Bring Google Tag Manager custom HTML tags into cfgov-refresh #3919

Merged
merged 3 commits into from
Mar 23, 2018

Conversation

anselmbradford
Copy link
Member

Google Tag Manager (GTM) introduces JS code to the site without code review and tests.
GTM also includes youtube code that may be the source of [ghe]/CFGOV/platform/issues/166.

This PR brings the existing code from GTM to this repo so that we can improve its quality and test coverage. This will run the scripts through webpack and make them available on the site at the static/apps/analytics-gtm/ URL, which will allow GTM to lazy load the scripts by utilizing the global jsl() function we already use to load other scripts.

Additions

  • GTM custom HTML tags from Google Tag Manager dashboard.

Changes

  • Moves javascript_loader jinja block to the head above the analytics code imports.

Testing

  1. JS should load on the site as before.
  2. Edit unprocessed/apps/analytics-gtm/bah-check-rates-listeners.js to add console.log( 'OAHRCAnalytics loaded!' ); on line 2.
  3. Run gulp build.
  4. Add this:
<script>
   jsl(['/static/apps/analytics-gtm/js/bah-check-rates-listeners.js']);
</script>

To line 150 of base-common.html
5. Verify you see OAHRCAnalytics loaded! in the dev console when loading a page. Note: there will be an error too, unless you're viewing on the appropriate OAH page. This doesn't matter though, we just want to know that it loads.

Notes

  • After this goes to production the custom HTML tags in GTM should be updated with jsl(['/static/apps/analytics-gtm/js/[script to load].js']);, where [script to load] is the custom HTML tag to load.

@anselmbradford
Copy link
Member Author

Note that edits to the actual scripts in unprocessed/apps/analytics-gtm/js/ shouldn't happen in this PR, though feel free to comment on things you see. Those scripts are not put live in this PR and are duplicates of what is currently delivered to the site. When they go live and the GTM copy is removed we can then refine them here.

@anselmbradford anselmbradford force-pushed the analytics_proxy branch 2 times, most recently from 07228c8 to 75971f4 Compare March 21, 2018 22:34
@@ -134,6 +134,14 @@
If you come across a script that makes a convincing case to be included in
the head, then file an issue or PR to discuss including it.
#}
{% block javascript_loader %}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just recombine base-common.html with base.html. Most of the components in base.html can be overridden.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! Though let's give that its own PR.

@sebworks
Copy link
Contributor

This is really good; one of my favorite PRs. How does this affect apps which aren't in cfgov-refresh?

Settings for webpack JavaScript bundling system.
========================================================================== */

const BROWSER_LIST = require( '../../../../config/browser-list-config' );
Copy link
Contributor

@sebworks sebworks Mar 22, 2018

Choose a reason for hiding this comment

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

Use import statements here.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed in chat, webpack config is not run through babel so imports aren't allowed yet.

@anselmbradford
Copy link
Member Author

anselmbradford commented Mar 22, 2018

How does this affect apps which aren't in cfgov-refresh?

That's a good question. So what about this:

  • Take the inline jsl script and move it to its own unminified JS file, which is run through webpack and minified like everything else.

  • Take the minified standalone jsl script created above and include it inline in the base template (like is done with modernizr).

  • In GTM have a script that tries to load the analytics script ('/static/apps/analytics-gtm/js/bah-check-rates-listeners.js' or whatever) locally, and if not found (if the page is not under consumerfinance.gov), have it load the jsl script from a consumerfinance.gov absolute URL and then use the loaded jsl script to load the analytics script from an absolute consumerfinance.gov URL as well.

jsl(['https://www.consumerfinance.gov/static/apps/analytics-gtm/js/bah-check-rates-listeners.js'])

I'm not sure if we'd run into cross-origin issues there though?

@mistergone
Copy link
Member

This is great!

As a note, I think the main apps with custom analytics outside cfgov-refresh are (parts of) Paying for College and Before You Claim.

Copy link
Member

@ascott1 ascott1 left a comment

Choose a reason for hiding this comment

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

An enthusiastic 👍 for moving these from GTM's dashboard to source control.

It looks like a lot of these reference jQuery. In talking with @anselmbradford these are OaH scripts, which still relies on jQuery.

@anselmbradford
Copy link
Member Author

I'm going to merge this and then test in GTM next deploy and address options for #3919 (comment)

@anselmbradford anselmbradford merged commit 2d3bc40 into master Mar 23, 2018
@anselmbradford anselmbradford deleted the analytics_proxy branch March 23, 2018 18:05
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.

None yet

4 participants