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

[DISCUSS] Switch from JS custom variables to HTML meta tags #117

Closed
wants to merge 2 commits into from

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Mar 5, 2015

Do not merge, discussion only

Append page metadata as 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 intention is that Javascript within static will handle the setting of custom dimensions and variables for analytics, based on these meta tags.

Example output

<meta name="format" content="homepage">
<meta name="organisations" content="<D19><D20>">

Alternatives

There are some alternatives to this approach we've considered:

  • Using data- attributes on a tag, possibly a <script>
  • Using raw JS variables
  • Using meta tags that use microdata itemscope and itemprop properties (the scope would have to be the <head>)
  • Using a single meta tag with data- attributes

There's a good discussion about the pros and cons of these different techniques here:
http://stackoverflow.com/questions/10533764/best-practice-for-meta-data-in-a-html-document

The meta element can represent document-level metadata with the name attribute
http://www.w3.org/TR/html5/document-metadata.html#the-meta-element

Custom meta names can be registered/proposed on the whatwg wiki, many of these appear to be website specific and namespaced:
https://wiki.whatwg.org/wiki/MetaExtensions

fofr added 2 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.
@benilovj
Copy link
Contributor

@benilovj benilovj commented Mar 5, 2015

👍 from me, but it'd be good to get another pair of eyes on this since I paired with Paul on some of this

@bradwright
Copy link
Contributor

@bradwright bradwright commented Mar 5, 2015

I'm all for something like this, although I'd like it to be more generally machine readable than using custom meta elements (which is tied to our implementation). Looking at schema.org I can't see an obvious way to use that, so it'd be good to explore what some of the options look like.

@bradwright
Copy link
Contributor

@bradwright bradwright commented Mar 5, 2015

Also I think we should not name organisations as it is, because what we're actually sending is GA specific - we should reserve organisations for human readable variations. Maybe analytics-organisations?

@fofr
Copy link
Contributor Author

@fofr fofr commented Mar 5, 2015

@bradleywright We looked at schema.org, open graph and microformats for patterns we could use for this data but came up short.

@edds
Copy link
Contributor

@edds edds commented Mar 5, 2015

Thinking out loud: If we are going to move these to just html tags I wonder if it is worth moving this out of slimmer like this and use a component (network shared template). Currently this is mostly just taking a key, value array and producing some markup, this is what components are really good at. It would also then remove the confusion as to where the logic for this exists and make it easier for someone to work out what is going on as it would just be erb rather than Nokogiri.

@bradwright
Copy link
Contributor

@bradwright bradwright commented Mar 5, 2015

@edds good thinking.

We could also potentially overload the metadata component to include some of these as data- attributes...?

@benilovj
Copy link
Contributor

@benilovj benilovj commented Mar 5, 2015

I wonder if it is worth moving this out of slimmer like this and use a component (network shared template).

I think this is the right thing to do; could we do it as a subsequent change straight after this one? We need to get to a point of parity between GA Classic and Universal Analytics so that we can compare like for like.

@edds
Copy link
Contributor

@edds edds commented Mar 5, 2015

@benilovj: I am not sure I see the merit in using this as a halfway if we want to eventually move to a component. To make this change go out you will need to bump slimmer in every application and redeploy them all. To move to a component you will then have to do that all over again. If we spend a small amount more time making sure we work out the right way of doing this up front you will only have to deploy every application once.

@benilovj
Copy link
Contributor

@benilovj benilovj commented Mar 5, 2015

To make this change go out you will need to bump slimmer in every application and redeploy them all. To move to a component you will then have to do that all over again.

Ok fair enough

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Mar 5, 2015

I think the overall approach - separating our metadata from GA - is 👍. It supports other uses, like the bookmarklet to get a format for a page, in a better way.

I've gone off data-attr on the analytics script (as there's still some coupling with JS). Using <metadata> looks like a good idea, it feels the most webby. Using unnamespaced keys, unless they match the use of a standardised key, feels like it might cause problems (but thats a reckon).

@fofr
Copy link
Contributor Author

@fofr fofr commented Mar 5, 2015

The component idea sounds good to me, though I'm unsure about the implementation. Would each app need to include the component partial, and then pass its data to it directly (rather than extracting it from slimmer headers/artefacts)?

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Mar 5, 2015

Apps explicitly passing to the component values Organisations, WorldLocations, Format and ResultCount makes sense - that is effectively what we're doing via headers.

Section and NeedID would need to be passed everywhere, which would be some duplication, but i'm not sure how big of a deal that would be. Depends if more might be added in the future?

@tombye
Copy link

@tombye tombye commented Mar 5, 2015

This is great 👍 I agree with @dsingleton about preferring <meta> tags over data- attributes as I think we'd gain more from treating the data as a public part of the document.

I can't comment on the component implementation but if regards key names I'd echo @bradleywright on use of an analytics- prefix to remove any ambiguity.

@edds
Copy link
Contributor

@edds edds commented Mar 5, 2015

I picture every app calling something like:

<%= render partial: "govuk_component/analytics_data", locals: {
    section: "", 
    need_ids: ["", "", ...],
    organisations: ["", "", ...],
    world_locations: ["", "", ...],
    format: "",
    search_result_count: "",
} %>

In the head of all their pages. This would then use the components code to load a template and render the data as it sees fit (weather as an element with data attributes or meta tags or a script tag with a config block).

As the component template would be in static next to the analytics javascript updating the way it looks for the the data could be done in a single pull request on, and deploy of, static. There would be duplication across each application but it makes it much easier to work out what is going on in the future as you can very easily see where the data is being set and how it gets there.

@benilovj
Copy link
Contributor

@benilovj benilovj commented Mar 9, 2015

@fofr and I just spent half a day prototyping a components approach within frontend.

While using components is probably the right thing to do for analytics, it'll require considerable work to unpick the slimmer integration just within frontend (eg here), and we haven't looked at the other apps yet.

Given that this current PR doesn't make the code worse than it was before, and removes the coupling between metadata and analytics, I believe that we should defer the component work until later, even at the cost of having to bump slimmer one extra time.

@edds
Copy link
Contributor

@edds edds commented Mar 9, 2015

Just had a chat with @fofr and @benilovj about this. We are in agreement that doing something similar to this pull request and making slimmer still handle adding the data to the page is the right thing to do in the short term. We can then look into moving to a component or some other kind of shared template for this later (probably at the point we want to start adding any new global custom variables).

@fofr
Copy link
Contributor Author

@fofr fofr commented Mar 10, 2015

Closing this discussion. A new PR will be opened for merging something similar to this.

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

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