From d0243764a2d2092e8e31d8acf0264766e2060439 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 26 Jun 2021 22:51:25 +0200 Subject: [PATCH] Extract component to render a list of links This way it's easier to refactor it and/or change it. Back in commit c156621a4 I wrote: > Generally speaking, I'm not a big fan of helpers, but there are > methods which IMHO qualify as helpers when (...) many Rails helpers, > like `tag`, follow these principles. It's time to modify these criteria a little bit. In some situations, it's great to have a helper method so it can be easily used in view (like `link_to`). However, from the maintenance point of view, helper methods are usually messy because extracting methods requires making sure there isn't another helper method with that name. So we can use the best part of these worlds and provide a helper so it can be easily called from the view, but internally make that helper render a component and enjoy the advantages associated with using an isolated Ruby class. --- app/components/shared/link_list_component.rb | 26 ++++++ app/helpers/link_list_helper.rb | 14 +-- .../shared/link_list_component_spec.rb | 93 +++++++++++++++++++ spec/helpers/link_list_helper_spec.rb | 91 ------------------ 4 files changed, 120 insertions(+), 104 deletions(-) create mode 100644 app/components/shared/link_list_component.rb create mode 100644 spec/components/shared/link_list_component_spec.rb delete mode 100644 spec/helpers/link_list_helper_spec.rb diff --git a/app/components/shared/link_list_component.rb b/app/components/shared/link_list_component.rb new file mode 100644 index 000000000..840fafe47 --- /dev/null +++ b/app/components/shared/link_list_component.rb @@ -0,0 +1,26 @@ +class Shared::LinkListComponent < ApplicationComponent + attr_reader :links, :options + + def initialize(*links, **options) + @links = links + @options = options + end + + def render? + links.select(&:present?).any? + end + + def call + tag.ul(options) do + safe_join(links.select(&:present?).map do |text, url, current = false, **link_options| + tag.li(({ "aria-current": true } if current)) do + if url + link_to text, url, link_options + else + text + end + end + end, "\n") + end + end +end diff --git a/app/helpers/link_list_helper.rb b/app/helpers/link_list_helper.rb index 5af374fae..0850138fa 100644 --- a/app/helpers/link_list_helper.rb +++ b/app/helpers/link_list_helper.rb @@ -1,17 +1,5 @@ module LinkListHelper def link_list(*links, **options) - return "" if links.select(&:present?).empty? - - tag.ul(options) do - safe_join(links.select(&:present?).map do |text, url, current = false, **link_options| - tag.li(({ "aria-current": true } if current)) do - if url - link_to text, url, link_options - else - text - end - end - end, "\n") - end + render Shared::LinkListComponent.new(*links, **options) end end diff --git a/spec/components/shared/link_list_component_spec.rb b/spec/components/shared/link_list_component_spec.rb new file mode 100644 index 000000000..2db5e3012 --- /dev/null +++ b/spec/components/shared/link_list_component_spec.rb @@ -0,0 +1,93 @@ +require "rails_helper" + +describe Shared::LinkListComponent, type: :component do + it "renders nothing with an empty list" do + render_inline Shared::LinkListComponent.new + + expect(page).not_to have_css "ul" + end + + it "returns nothing with a list of nil elements" do + render_inline Shared::LinkListComponent.new(nil, nil) + + expect(page).not_to have_css "ul" + end + + it "generates a list of links" do + render_inline Shared::LinkListComponent.new( + ["Home", "/"], ["Info", "/info"], class: "menu" + ) + list = page.find("body").native.inner_html + + expect(list).to eq '" + end + + it "accepts anchor tags" do + render_inline Shared::LinkListComponent.new( + 'Home'.html_safe, ["Info", "/info"], class: "menu" + ) + list = page.find("body").native.inner_html + + expect(list).to eq '" + end + + it "accepts options for links" do + render_inline Shared::LinkListComponent.new( + ["Home", "/", class: "root"], ["Info", "/info", id: "info"] + ) + + expect(page).to have_css "a", count: 2 + expect(page).to have_css "a.root", count: 1, exact_text: "Home" + expect(page).to have_css "a#info", count: 1, exact_text: "Info" + end + + it "ignores nil entries" do + render_inline Shared::LinkListComponent.new( + ["Home", "/", class: "root"], nil, ["Info", "/info", id: "info"] + ) + + expect(page).to have_css "li", count: 2 + expect(page).to have_css "a.root", count: 1, exact_text: "Home" + expect(page).to have_css "a#info", count: 1, exact_text: "Info" + end + + it "ignores empty entries" do + render_inline Shared::LinkListComponent.new( + ["Home", "/", class: "root"], "", ["Info", "/info", id: "info"] + ) + + expect(page).to have_css "li", count: 2 + expect(page).to have_css "a.root", count: 1, exact_text: "Home" + expect(page).to have_css "a#info", count: 1, exact_text: "Info" + end + + it "accepts an optional condition to check the active element" do + render_inline Shared::LinkListComponent.new( + ["Home", "/", false], + ["Info", "/info", true], + ["Help", "/help"] + ) + + expect(page).to have_css "li", count: 3 + expect(page).to have_css "li[aria-current='true']", count: 1, exact_text: "Info" + end + + it "allows passing both the active condition and link options" do + render_inline Shared::LinkListComponent.new( + ["Home", "/", false, class: "root"], + ["Info", "/info", true, id: "info"], + ["Help", "/help", rel: "help"] + ) + + expect(page).to have_css "li", count: 3 + expect(page).to have_css "li[aria-current='true']", count: 1, exact_text: "Info" + + expect(page).to have_css "a.root", count: 1, exact_text: "Home" + expect(page).to have_css "a#info", count: 1, exact_text: "Info" + expect(page).to have_css "a[rel='help']", count: 1, exact_text: "Help" + end +end diff --git a/spec/helpers/link_list_helper_spec.rb b/spec/helpers/link_list_helper_spec.rb deleted file mode 100644 index 7626d0198..000000000 --- a/spec/helpers/link_list_helper_spec.rb +++ /dev/null @@ -1,91 +0,0 @@ -require "rails_helper" - -describe LinkListHelper do - describe "#link_list" do - it "returns an empty string with an empty list" do - expect(helper.link_list).to eq "" - end - - it "returns nothing with a list of nil elements" do - expect(helper.link_list(nil, nil)).to eq "" - end - - it "generates a list of links" do - list = helper.link_list(["Home", "/"], ["Info", "/info"], class: "menu") - - expect(list).to eq '' - expect(list).to be_html_safe - end - - it "accepts anchor tags" do - list = helper.link_list(link_to("Home", "/"), ["Info", "/info"], class: "menu") - - expect(list).to eq '' - expect(list).to be_html_safe - end - - it "accepts options for links" do - render helper.link_list(["Home", "/", class: "root"], ["Info", "/info", id: "info"]) - - expect(page).to have_css "a", count: 2 - expect(page).to have_css "a.root", count: 1, exact_text: "Home" - expect(page).to have_css "a#info", count: 1, exact_text: "Info" - end - - it "ignores nil entries" do - render helper.link_list(["Home", "/", class: "root"], nil, ["Info", "/info", id: "info"]) - - expect(page).to have_css "li", count: 2 - expect(page).to have_css "a.root", count: 1, exact_text: "Home" - expect(page).to have_css "a#info", count: 1, exact_text: "Info" - end - - it "ignores empty entries" do - render helper.link_list(["Home", "/", class: "root"], "", ["Info", "/info", id: "info"]) - - expect(page).to have_css "li", count: 2 - expect(page).to have_css "a.root", count: 1, exact_text: "Home" - expect(page).to have_css "a#info", count: 1, exact_text: "Info" - end - - it "accepts an optional condition to check the active element" do - render helper.link_list( - ["Home", "/", false], - ["Info", "/info", true], - ["Help", "/help"] - ) - - expect(page).to have_css "li", count: 3 - expect(page).to have_css "li[aria-current='true']", count: 1, exact_text: "Info" - end - - it "allows passing both the active condition and link options" do - render helper.link_list( - ["Home", "/", false, class: "root"], - ["Info", "/info", true, id: "info"], - ["Help", "/help", rel: "help"] - ) - - expect(page).to have_css "li", count: 3 - expect(page).to have_css "li[aria-current='true']", count: 1, exact_text: "Info" - - expect(page).to have_css "a.root", count: 1, exact_text: "Home" - expect(page).to have_css "a#info", count: 1, exact_text: "Info" - expect(page).to have_css "a[rel='help']", count: 1, exact_text: "Help" - end - end - - attr_reader :content - - def render(content) - @content = content - end - - def page - Capybara::Node::Simple.new(content) - end -end