Skip to content

Commit

Permalink
Remove new tab functionality
Browse files Browse the repository at this point in the history 
Despite this being a good move the implementation breaks because of
the close ties to Rails' parameter names. We have to do a lot of
juggling of params to make them flexible enough to accept either a name
and href, a href and block, a name and Rails action/controller via
options etc.

Adding a named param `new_tab` results in the unenclosed
options/extra_options hash being interpreted as a named param, meaning
users of this library would need to go through their app and make
changes like this:

Before:

= link_to("some page", "/some/path", class: "thing")

After:

= link_to("some page", "/some/path", { class: "thing" })

Otherwise, class is treated as a named arg rather than being the options hash.
  • Loading branch information
@peteryates
peteryates committed Feb 10, 2023
1 parent 84937d4 commit 28eafec
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 125 deletions.
36 changes: 8 additions & 28 deletions app/helpers/govuk_link_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,6 @@ module GovukLinkHelper
warning: "govuk-button--warning",
}.freeze

NEW_TAB_ATTRIBUTES = {
target: "_blank",
rel: "noreferrer noopener"
}.freeze

def govuk_link_classes(*styles, default_class: 'govuk-link')
if (invalid_styles = (styles - LINK_STYLES.keys)) && invalid_styles.any?
fail(ArgumentError, "invalid styles #{invalid_styles.to_sentence}. Valid styles are #{LINK_STYLES.keys.to_sentence}")
Expand All @@ -38,14 +33,14 @@ def govuk_button_classes(*styles, default_class: 'govuk-button')
[default_class] + BUTTON_STYLES.values_at(*styles).compact
end

def govuk_link_to(name = nil, options = nil, extra_options = {}, new_tab: false, &block)
def govuk_link_to(name = nil, options = nil, extra_options = {}, &block)
extra_options = options if block_given?
html_options = build_html_options(extra_options, new_tab: new_tab)
html_options = build_html_options(extra_options)

if block_given?
link_to(name, html_options, &block)
else
link_to(prepare_link_text(name, new_tab), options, html_options)
link_to(name, options, html_options)
end
end

Expand All @@ -71,15 +66,15 @@ def govuk_button_to(name = nil, options = nil, extra_options = {}, &block)
end
end

def govuk_button_link_to(name = nil, options = nil, extra_options = {}, new_tab: false, &block)
def govuk_button_link_to(name = nil, options = nil, extra_options = {}, &block)
extra_options = options if block_given?
html_options = GovukComponent::StartButtonComponent::LINK_ATTRIBUTES
.merge build_html_options(extra_options, style: :button, new_tab: new_tab)
.merge build_html_options(extra_options, style: :button)

if block_given?
link_to(name, html_options, &block)
else
link_to(prepare_link_text(name, new_tab), options, html_options)
link_to(name, options, html_options)
end
end

Expand All @@ -96,24 +91,19 @@ def govuk_breadcrumb_link_to(name = nil, options = nil, extra_options = {}, &blo

private

def build_html_options(provided_options, style: :link, new_tab: false)
def build_html_options(provided_options, style: :link)
element_styles = { link: LINK_STYLES, button: BUTTON_STYLES }.fetch(style, {})

# we need to take a couple of extra steps here because we don't want the style
# params (inverse, muted, etc) to end up as extra attributes on the link.

remaining_options = new_tab_options(new_tab)
.deep_merge_html_attributes(remove_styles_from_provided_options(element_styles, provided_options))
remaining_options = remove_styles_from_provided_options(element_styles, provided_options)

style_classes = build_style_classes(style, extract_styles_from_provided_options(element_styles, provided_options))

combine_attributes(remaining_options, class_name: style_classes)
end

def new_tab_options(new_tab)
new_tab ? NEW_TAB_ATTRIBUTES : {}
end

def build_style_classes(style, provided_options)
keys = *provided_options&.keys

Expand Down Expand Up @@ -143,16 +133,6 @@ def remove_styles_from_provided_options(styles, provided_options)

provided_options&.except(*styles.keys)
end

def prepare_link_text(text, new_tab)
return text unless new_tab

new_tab_text = new_tab.is_a?(String) ? new_tab : Govuk::Components.config.default_link_new_tab_text

return text if new_tab_text.blank?

%(#{text} #{new_tab_text})
end
end

ActiveSupport.on_load(:action_view) { include GovukLinkHelper }
17 changes: 0 additions & 17 deletions guide/content/helpers/link.slim
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,6 @@ markdown:

[0]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a

== render('/partials/example.*',
caption: "Links that open in a new tab",
code: govuk_link_to_new_tab) do

markdown:
You can use the `new_tab` parameter to automatically set the `target` and
`rel` [attributes as suggested by the design system][0].

When the link text is passed in as a string argument, 'opens in new tab' is
added to the end automatically. No suffix is added when the helper is used
in block mode.

The default text can be set using the `default_link_new_tab_text` configuration option
and it can be suppressed by setting it to an empty string.

[0]: https://design-system.service.gov.uk/styles/typography/#opening-links-in-a-new-tab

== render('/partials/example.*',
caption: "Inverse hyperlink",
code: govuk_link_to_inverse,
Expand Down
80 changes: 0 additions & 80 deletions spec/helpers/govuk_link_helper_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,45 +111,6 @@

it { is_expected.to have_tag('a', with: { href: link_url, class: %w(green govuk-link--no-underline) }, text: link_text) }
end

describe "opening links in new tabs" do
context "when new_tab: true" do
let(:default_new_tab_text) { "(opens in new tab)" }
subject { govuk_link_to(link_text, link_params, new_tab: true) }

it { is_expected.to have_tag('a', with: { href: link_url, class: %w(govuk-link), target: "_blank", rel: "noreferrer noopener" }, text: "#{link_text} #{default_new_tab_text}") }
end

context "when new_tab: '(opens in new window)'" do
let(:overridden_new_tab_text) { "(opens in new window)" }
subject { govuk_link_to(link_text, link_params, new_tab: overridden_new_tab_text) }

it { is_expected.to have_tag('a', with: { href: link_url, class: %w(govuk-link), target: "_blank", rel: "noreferrer noopener" }, text: "#{link_text} #{overridden_new_tab_text}") }
end

context "when new_tab is an empty string" do
let(:overridden_new_tab_text) { "" }
subject { govuk_link_to(link_text, link_params, new_tab: overridden_new_tab_text) }

it "appends nothing to the string" do
expect(subject).to have_tag('a', with: { href: link_url, class: %w(govuk-link), target: "_blank", rel: "noreferrer noopener" }, text: link_text)
end
end

context "when custom 'rel' and 'target' values are provided" do
let(:custom_rel) { { rel: "help" } }
let(:custom_target) { { target: "new" } }
subject { govuk_link_to(link_text, link_params, { **custom_rel, **custom_target }, new_tab: true) }

it "replaces the default target value with the provided one" do
expect(subject).to have_tag('a', with: { rel: "noreferrer noopener help" })
end

it "appends the provided rel value to 'noreferrer' and 'noopener'" do
expect(subject).to have_tag('a', with: { target: 'new' })
end
end
end
end

describe "#govuk_mail_to" do
Expand Down Expand Up @@ -313,47 +274,6 @@
expect(subject).to have_tag("a", with: { href: button_link_url, class: %w(govuk-button yellow) }, text: button_link_text)
end
end

describe "opening links in new tabs" do
let(:link_params) { { controller: :some_controller, action: :some_action } }

context "when new_tab: true" do
let(:default_new_tab_text) { "(opens in new tab)" }
subject { govuk_button_link_to(button_link_text, link_params, new_tab: true) }

it { is_expected.to have_tag('a', with: { href: button_link_url, class: %w(govuk-button), target: "_blank", rel: "noreferrer noopener" }, text: "#{button_link_text} #{default_new_tab_text}") }
end

context "when new_tab: '(opens in new window)'" do
let(:overridden_new_tab_text) { "(opens in new window)" }
subject { govuk_button_link_to(button_link_text, link_params, new_tab: overridden_new_tab_text) }

it { is_expected.to have_tag('a', with: { href: button_link_url, class: %w(govuk-button), target: "_blank", rel: "noreferrer noopener" }, text: "#{button_link_text} #{overridden_new_tab_text}") }
end

context "when new_tab is an empty string" do
let(:overridden_new_tab_text) { "" }
subject { govuk_button_link_to(button_link_text, link_params, new_tab: overridden_new_tab_text) }

it "appends nothing to the string" do
expect(subject).to have_tag('a', with: { href: button_link_url, class: %w(govuk-button), target: "_blank", rel: "noreferrer noopener" }, text: button_link_text)
end
end

context "when custom 'rel' and 'target' values are provided" do
let(:custom_rel) { { rel: "help" } }
let(:custom_target) { { target: "new" } }
subject { govuk_button_link_to(button_link_text, link_params, { **custom_rel, **custom_target }, new_tab: true) }

it "replaces the default target value with the provided one" do
expect(subject).to have_tag('a', with: { rel: "noreferrer noopener help" })
end

it "appends the provided rel value to 'noreferrer' and 'noopener'" do
expect(subject).to have_tag('a', with: { target: 'new' })
end
end
end
end

describe "#govuk_breadcrumb_link_to" do
Expand Down

0 comments on commit 28eafec

Please sign in to comment.