From 28eafec70eca55dfadabad62790b3adc99421ec5 Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Fri, 10 Feb 2023 18:49:06 +0000 Subject: [PATCH] Remove new tab functionality 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. --- app/helpers/govuk_link_helper.rb | 36 +++--------- guide/content/helpers/link.slim | 17 ------ spec/helpers/govuk_link_helper_spec.rb | 80 -------------------------- 3 files changed, 8 insertions(+), 125 deletions(-) diff --git a/app/helpers/govuk_link_helper.rb b/app/helpers/govuk_link_helper.rb index 9bcec1e3..7d2eca5c 100644 --- a/app/helpers/govuk_link_helper.rb +++ b/app/helpers/govuk_link_helper.rb @@ -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}") @@ -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 @@ -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 @@ -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 @@ -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 } diff --git a/guide/content/helpers/link.slim b/guide/content/helpers/link.slim index 9b4b0b82..30df0038 100644 --- a/guide/content/helpers/link.slim +++ b/guide/content/helpers/link.slim @@ -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, diff --git a/spec/helpers/govuk_link_helper_spec.rb b/spec/helpers/govuk_link_helper_spec.rb index be546cce..0603eb3e 100644 --- a/spec/helpers/govuk_link_helper_spec.rb +++ b/spec/helpers/govuk_link_helper_spec.rb @@ -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 @@ -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