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

Switch from JS custom variables to HTML meta tags #119

Merged
merged 5 commits into from Mar 12, 2015
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Mar 11, 2015

Append page metadata as govuk- namespaced meta tags within the <head> tag, as an alternative to setting Google custom variables within a script tag. This removes Google specific implementation details (page level scope, custom variable name, custom variable slot), the GOVUK.Analytics namespace and an inline <script> tag.

Javascript within static will handle the setting of custom dimensions and variables for analytics, based on these meta tags. A WIP example of that can be found here: alphagov/static@3c87c32

Example output

<meta name="govuk:format" content="news">
<meta name="govuk:analytics:organisations" content="<D19><D20>">
<meta name="govuk:analytics:world-locations" content="<WL3>">

Prior discussion: #117

fofr added 4 commits Mar 4, 2015
Append page metadata as meta tags as an alternative to setting Google
custom variables within a script tag.

* Removes Google specific implementation details (page level scope,
custom variable name, custom variable slot)
* Javascript in static will handle the setting of custom dimensions and
variables based on this data
Metadata is now being passed to templates in a data agnostic way,
without the GA specific implementation.
* Avoid clashes with defined meta names
* Make it easy for scripts to pull out GOVUK specific meta data
Indicate that their format is specific to analytics. This leaves the
non `analytics` versions for human readable values.
@benilovj
Copy link
Contributor

@benilovj benilovj commented Mar 11, 2015

👍

@bradwright
Copy link
Contributor

@bradwright bradwright commented Mar 11, 2015

👍 for the proposed HTML. Not reviewed the code though.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Mar 11, 2015

Only commenting on the meta tags, as i've not had time to read the bulk of the changes.

The approach is 👍 but i'd recommend using different character for namespacing, so it's clear what is a many-word thing, vs a namespace. eg:

<meta name="govuk-analytics-world-locations" content="<WL3>">

Would be clearer as one of:

<meta name="govuk:analytics:world-locations" content="<WL3>">
<meta name="govuk.analytics.world-locations" content="<WL3>">

I don't have a strong preference on . vs :, Dublin Core uses ., which is pretty wide spread, but Open Graph uses :, which is probably ore recognisable.

I know it's a little pedantic, but I think the readability is worth it 🍍

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Mar 11, 2015

Read the rest, the rest generally looks good 👍

It's nice to be rid of the #ga-param hack, but there are still a few other references:

  • Slimmer test template can just be updated, I think which you totally did
  • Smart Answer compare task, this will need updating when, slimmer is upgraded on SA, to ignore the meta tags instead. Nothing needs doing now, other than giving smart answer folks a heads up (and maybe creating an issue to track it/add visibility?)
Be clearer about what’s a namespace and what’s a hyphenated name.
@fofr
Copy link
Contributor Author

@fofr fofr commented Mar 12, 2015

@dsingleton I agree about the namespacing, I've switched to using : in 430b7ff

@benilovj
Copy link
Contributor

@benilovj benilovj commented Mar 12, 2015

@bradleywright @edds do either of you have any objections to the chosen namespacing?

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Mar 12, 2015

I'm happy with @fofr's name-spacing changes, but would like someone else to agree before merging.

@bradwright
Copy link
Contributor

@bradwright bradwright commented Mar 12, 2015

All good here.

dsingleton added a commit that referenced this pull request Mar 12, 2015
Switch from JS custom variables to HTML meta tags
@dsingleton dsingleton merged commit 05a9a87 into master Mar 12, 2015
1 check passed
1 check passed
default "Build #92 succeeded on Jenkins"
Details
@dsingleton dsingleton deleted the metadata-inserter branch Mar 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.