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.

Sign up for GitHub

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

Suppress subscription components heading #804

Merged
merged 2 commits into from
Mar 28, 2019
Merged

Suppress subscription components heading #804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e73b682
Suppress subscription components heading
aliuk2012 Mar 27, 2019
3fa4132
Add changelog entry
aliuk2012 Mar 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Control when checkboxes change event sends Google Analytics tracking info (PR #801)
* Updates govuk-frontend from 2.5.1 to 2.9.0 (PR #794)
* Adds small form option to subscription component (PR #803)
* Suppress subscription components heading (PR #804)

## 16.8.0

Expand Down
5 changes: 4 additions & 1 deletion app/views/govuk_publishing_components/components/_subscription-links.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,13 @@
css_classes << (shared_helper.get_margin_bottom) unless local_assigns[:margin_bottom] == 0
css_classes << brand_helper.brand_class
data = {"module": "gem-toggle"} if sl_helper.feed_link_box_value
hide_heading ||= false
Copy link
Contributor

@alex-ju alex-ju Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if we could turn this into a positive flag show_heading ||= true which can be turned off. It's a matter of flavour, but I always find confusing setting things to true to disable something. Especially because we don't know why we set this <h2> implicitly on this component and we may consider dropping it at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried using positive flags before but it doesn't work with the way we set the defaults using ||= operator.

TBH, I would have liked to have simply remove the h2 but I just don't have time to update and test the other repos.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a positive flag, but we'd have to go and change every existing use of the component.

Copy link
Contributor

@alex-ju alex-ju Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andysellick: I don't think so if we're setting the default to true.
I'm not very opinionated, whatever does the job 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex-ju that's the problem Al was talking about. If you set the default to true, like this: show_heading ||= true when you pass through 'false' it defaults to true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, familiar with this situation, but we dealt with it in other components with a conditional statement.

%>
<% if sl_helper.component_data_is_valid? %>
<%= tag.section class: css_classes, data: data do %>
<h2 class="gem-c-subscription-links__hidden-header visuallyhidden"><%= t("govuk_component.subscription_links.subscriptions", default: "Subscriptions") %></h2>
<% unless hide_heading %>
<h2 class="gem-c-subscription-links__hidden-header visuallyhidden"><%= t("govuk_component.subscription_links.subscriptions", default: "Subscriptions") %></h2>
<% end %>
<ul
class="gem-c-subscription-links__list<%= ' gem-c-subscription-links__list--small' if local_assigns[:small_form] == true %>"
<%= "data-module=track-click" if sl_helper.tracking_is_present? %>
Expand Down
11 changes: 10 additions & 1 deletion app/views/govuk_publishing_components/components/docs/subscription-links.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
name: Subscription links
description: Links to ‘Get email alerts’ and ‘Subscribe to feed’
body: |
<strong>NOTE: This component includes a h2 heading by default but can be suppressed by using `hide_heading` option (see below)<strong>
accessibility_criteria: |
Icons in subscription links must be presentational and ignored by screen readers.

Expand Down Expand Up @@ -77,4 +79,11 @@ examples:
email_signup_link: '/foreign-travel-advice/singapore/email-signup'
feed_link: '/foreign-travel-advice/singapore.atom'
small_form: true

without_heading:
description: |
By default the component includes an h2 heading. The component could be used anywhere on the page and could mean
that it produces invalid markup or make the site unaccessible.
data:
email_signup_link: '/foreign-travel-advice/singapore/email-signup'
feed_link: '/foreign-travel-advice/singapore.atom'
hide_heading: true
17 changes: 17 additions & 0 deletions spec/components/subscription_links_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,21 @@ def component_name
render_component(email_signup_link: 'email-signup', feed_link: 'singapore.atom', small_form: true)
assert_select ".gem-c-subscription-links__list--small"
end

describe 'component heading' do
it 'renders a heading by default' do
render_component(email_signup_link: 'email-signup', feed_link: 'singapore.atom')
assert_select "h2.gem-c-subscription-links__hidden-header"
end

it 'renders a heading by default' do
render_component(email_signup_link: 'email-signup', feed_link: 'singapore.atom', hide_heading: false)
assert_select "h2.gem-c-subscription-links__hidden-header"
end

it 'renders without a heading' do
render_component(email_signup_link: 'email-signup', feed_link: 'singapore.atom', hide_heading: true)
assert_select "h2.gem-c-subscription-links__hidden-header", false
end
end
end