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 format and need-ids to analytics component #674

Merged
merged 3 commits into from Nov 10, 2015

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Nov 9, 2015

https://trello.com/c/2LJAtceC/156-new-world-analytics-component-medium
Allow format and Need IDs to be tracked to Google Analytics through the existing component.

Matches the format that "old world" frontends are using via Slimmer:
https://github.com/alphagov/slimmer/blob/master/lib/slimmer/processors/metadata_inserter.rb

cc @jamiecobbett @dsingleton

fofr added 2 commits Nov 9, 2015
Split component into two, a top part that deals with the logic and a
second part that renders HTML.
Allow more types of metadata to be tracked in the new world.
@@ -2,19 +2,27 @@
# We have to call deep_symbolize_keys because we're often dealing with a
# parsed JSON document which will have string keys by default, but our
# components use symbol keys and we want consistency.
links_hash = content_item.to_h.deep_symbolize_keys[:links] || {}
content_item_hash = content_item.to_h.deep_symbolize_keys
links_hash = content_item_hash[:links] || {}

This comment has been minimized.

@benlovell

benlovell Nov 10, 2015
Contributor

Hash#fetch(:key, {}) may be preferable.

This comment has been minimized.

@jamiecobbett

jamiecobbett Nov 10, 2015
Contributor

We're keeping the language constructs used in these templates as generic as possible so that they could be ported to another templating language - @dsingleton can explain the rationale for that.

This comment has been minimized.

@jamiecobbett

jamiecobbett Nov 10, 2015
Contributor

Having said that, we're using map and join...

This comment has been minimized.

@dsingleton

dsingleton Nov 10, 2015
Contributor

The distinction is a bit fuzzy at the moment, but inside the "presenter" bit (eg, the complex processing of content_item) we can use any ERB features, as that is GOV.UK specific. We should avoid them in the "view" bit, which should be translatable in to other (simpler) languages, like mustache. The more ERB features we use in "views" the harder it is, as mentioned before.

As both the "presenter" and view" are in the same file, and we don't transpile the "views" yet, this is a bit obtuse. Sorry. Hopefully this will be covered better in some component principles documentation, when I have time to write it :(

In the mean time, fetch isn't a big deal in either place, as it's semantically equivalent to hash[:key] || "default", right? So both are translatable. We've avoided fetch in (most) other view templates to avoid encouraging use of more complex usage, which should hopefully be bettered covered by the principles, when we have them.

So i'd be fine with fetch being used like this, but we should avoid using it in the view, or other "view only" components (which is most of them).

Does that make sense?

This comment has been minimized.

@benlovell

benlovell Nov 10, 2015
Contributor

👍 seems legit.


meta_tags["govuk:format"] = content_item_hash[:format] if content_item_hash[:format]

need_ids = content_item_hash[:need_ids] || []

This comment has been minimized.

@benlovell

benlovell Nov 10, 2015
Contributor

Same here.

@dsingleton

This comment has been minimized.

Copy link
Contributor

@dsingleton dsingleton commented on e2b1c3a Nov 10, 2015

meta_tags is basically the arguments the "simple" component would take right? I think it might be worth making that official with fixtures in the component doc, and if we do that then it's worth a quick debate about the name.

if content_item
  <complex setup>
  meta_tags = {...}
end

<template bit>

Not a blocker for this PR, but adding it would make the intent clearer. What do you think?

@jamiecobbett
jamiecobbett reviewed Nov 10, 2015
View changes
app/views/govuk_component/analytics_meta_tags.raw.html.erb Outdated

organisations = []
organisations += links_hash[:organisations] || []
organisations += links_hash[:lead_organisations] || []
organisations += links_hash[:supporting_organisations] || []
organisations += links_hash[:worldwide_organisations] || []
organisations_content = organisations.map{ |link| "<#{link[:analytics_identifier]}>" }.join

This comment has been minimized.

@jamiecobbett

jamiecobbett Nov 10, 2015
Contributor

We should have a space before the { as per the styleguide, same below.

This comment has been minimized.

@dsingleton

dsingleton Nov 10, 2015
Contributor

Does our rubocop gem catch bits like that? Might be worth adding to static?

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Nov 10, 2015

LGTM, I would be happy to merge - though I think making meta_tags (by whatever name) part of the official API, with fixtures is worthwhile.

@fofr
Copy link
Contributor Author

@fofr fofr commented Nov 10, 2015

@dsingleton Aside: We discussed creating a simple meta tags component, but weren't sure whether there was a need. It's fairly standard HTML that isn't going to change between apps.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Nov 10, 2015

@dsingleton Aside: We discussed creating a simple meta tags component, but weren't sure whether there was a need. It's fairly standard HTML that isn't going to change between apps.

That makes sense, it's a pretty GOV.UK specific usage anyway, so 👍 to not generalising it.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Nov 10, 2015

👍 to merging, once the syntax issue is cleaned up.

@fofr fofr force-pushed the analytics-component-updates branch to 1a19443 Nov 10, 2015
@fofr
Copy link
Contributor Author

@fofr fofr commented Nov 10, 2015

I've updated to fix whitespace formatting.

jamiecobbett added a commit that referenced this pull request Nov 10, 2015
Add format and need-ids to analytics component
@jamiecobbett jamiecobbett merged commit 84a1db5 into master Nov 10, 2015
1 check passed
1 check passed
default "Build #682 succeeded on Jenkins"
Details
@jamiecobbett jamiecobbett deleted the analytics-component-updates branch Nov 10, 2015
@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Nov 10, 2015

tumblr_inline_n8turylmea1qew7bs

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.