-
Notifications
You must be signed in to change notification settings - Fork 17
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 supergroup sections to taxonomy navigation #994
Conversation
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.
I'd recommend squashing your commits into one and giving them slightly more descriptive messages so it's clear what's being changed
app/lib/services.rb
Outdated
@@ -10,4 +11,8 @@ def self.content_store | |||
disable_cache: true, | |||
) | |||
end | |||
|
|||
def self.rummager |
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.
I think once you rebase you won't need this as it's already been added for the Services work
@@ -71,6 +71,24 @@ def cache_control_public? | |||
!content_item.cache_control.private? | |||
end | |||
|
|||
def has_policy_and_engagement_links? |
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.
I think these should probably be moved out into policy_and_engagement.rb
as it's specific to that. Same with policy_and_engagement_links_content
config/application.rb
Outdated
config.autoload_paths += %W(#{config.root}/lib) | ||
config.autoload_paths += %W( | ||
#{config.root}/lib | ||
#{config.root}/app/services/supergroups |
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 is great because it follows the pattern in Collections, but our previous work on Services and News (I think) puts everything in lib for now. It may make sense to follow that pattern for now and perhaps create a card to move everything into /services
later.
<% if @content_item.has_policy_and_engagement_links? %> | ||
<%= render "govuk_publishing_components/components/highlight_boxes", { | ||
items: @content_item.policy_and_engagement_links_content } %> | ||
<% end %> |
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.
Missing newline at end of file 🙂 You can probably configure your editor to do this automatically.
@@ -24,3 +24,8 @@ | |||
|
|||
<%= render 'shared/sidebar_navigation' %> | |||
</div> | |||
|
|||
<% if @content_item.has_policy_and_engagement_links? %> |
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 logic looks good, but it needs to be added to shared/_taxonomy_navigation.html.erb
bf8c3aa
to
cae3074
Compare
08f7761
to
f817ab6
Compare
68062eb
to
de067d5
Compare
de067d5
to
b503d76
Compare
@vanitabarrett I've addressed a few things and added tracking. Do you think you could re-review if you have some time please? |
test/integration/answer_test.rb
Outdated
@@ -1,6 +1,8 @@ | |||
require 'test_helper' | |||
|
|||
class AnswerTest < ActionDispatch::IntegrationTest | |||
include RummagerHelpers |
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.
Is this needed here? Doesn't look like we're using it?
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.
I'll check that, thanks
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.
Removed, thanks
|
||
def data_attributes(base_path, link_text, index) | ||
{ | ||
track_category: data_module_label + "DocumentListClicked", |
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.
Looks like some of the tracking stuff won't work because the document list component doesn't have data-module="track-click"
, it requires you to wrap the document-list in an element with that attribute. Not sure how I didn't spot that with the services work, sorry! That'll need changing there too.
In the long term we probably just want to update the component to do this by default if someone passes tracking info, but it will require updating the other frontend apps.
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.
Boo! 👎
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.
I've wrapped the partial in the data-module now and it seems to work OK.
Adds guidance and regulation, and policy and engagement sections to the nav section at the footer of content pages. This is currently concealed from public view by an AB test. The navigation is visible if you're in the B sample of the ContentPagesNav test. The content page must itself be tagged to a live taxonomy, be rendered by this application and not be an HTML publication. Policy and engagement content is configured to get latest content. Guidance and regulation is retrieved by popularity (as determined by rummager). This PR is to get things generally close to the desired outcome and polish after a design review.
68a0407
to
04ae708
Compare
Wrapping the taxonomy block in a track-click data module activates link tracking. I've checked this in GA debug and it seems to do the trick. Also switching the guard clauses on each section from any? to present? as this checks whether hashes/arrays are nil or empty. (The services data can be nil)
04ae708
to
ca92c35
Compare
Adds guidance and regulation, and policy and engagement sections to the nav section at the footer of content pages.
They're both displayed as lists of items rather than highlight boxes.
The content page will show this navigation if they are tagged to a live taxonomy, be rendered by this application and not be an HTML publication (and have results to show)
This PR is to get things generally close to the desired outcome and then we'll polish after testing and a further design review.
Examples:
You will need to have an extension like ModHeader installed in your browser to see the B Variant. The values should be:
Request Header Name: GOVUK-ABTest-ContentPagesNav
Request Header Value: B
Component guide for this PR:
https://government-frontend-pr-994.herokuapp.com/component-guide