-
Notifications
You must be signed in to change notification settings - Fork 20
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
Subscription links add colour and tracking #299
Subscription links add colour and tracking #299
Conversation
feed_link_data = false | ||
feed_link_data_attributes ||= {} | ||
|
||
tracking_is_present = true if email_signup_link_data_attributes || !feed_link_data_attributes.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!feed_link_data_attributes.empty?
is a bit confusing. How about feed_link_data_attributes.any?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's much better.
@@ -1,39 +1,45 @@ | |||
<% | |||
email_signup_link ||= false | |||
email_signup_link_text ||= t("govuk_component.subscription_links.email_signup_link_text", default: "Get email alerts") | |||
email_signup_link_data_attributes ||= false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section at the top of the component is getting quite large now, and making it difficult to follow what's happening in the view. Can we move some of this logic out into a helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave it a go but by the time I've passed everything I need out to the helper and back again I'm not sure it actually saves any lines of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do something like we do for the related navigation component and just pass through the local assigns?
<% related_nav_helper = GovukPublishingComponents::Presenters::RelatedNavigationHelper.new(local_assigns) %>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanitabarrett ah good idea, I'd forgotten you can do that, thanks. Have moved the code into a helper.
Also possibly out of scope of this PR, but the component guide page for this component now errors because of a duplicate ID. It works when running on heroku etc, but stops you from viewing it locally. |
b74ff83
to
a2a0684
Compare
@vanitabarrett have fixed the duplicate id problem, I'd spotted it but was being lazy :) |
2d2e237
to
57b635f
Compare
57b635f
to
8f52856
Compare
8f52856
to
7e2f1d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helper refactor is much tidier! 👍 🎉
7e2f1d4
to
4fbaf8b
Compare
- expand feed link test to check for toggle module - improve template readability - test modules are not added unless needed - make test for data modules more specific - make id for feed box unique
4fbaf8b
to
f6fee99
Compare
Adds colour branding options and the ability to add tracking to the subscription links component.
Component guide: https://govuk-publishing-compon-pr-299.herokuapp.com/component-guide/subscription-links
Trello card: https://trello.com/c/NjgS3vym/55-modify-component-subscription-links