From 554bc926c7b9c739a88da79b3273046305aee54b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 24 Jun 2021 23:55:15 +0200 Subject: [PATCH 01/13] Get current locale from the lang HTML attribute The language attribute is present in all layouts since commit 025923ac4, so there's no need to use the language selector locale. Besides, it wouldn't work if there's only one locale and the language selector isn't shown. --- app/assets/javascripts/datepicker.js | 2 +- app/views/shared/_locale_switcher.html.erb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/datepicker.js b/app/assets/javascripts/datepicker.js index 8299833cd..1e483ea6a 100644 --- a/app/assets/javascripts/datepicker.js +++ b/app/assets/javascripts/datepicker.js @@ -17,7 +17,7 @@ App.Datepicker = { initialize: function() { var locale; - locale = $("#js-locale").data("current-locale"); + locale = document.documentElement.lang; $(".js-calendar").datepicker({ maxDate: "+0d" }); diff --git a/app/views/shared/_locale_switcher.html.erb b/app/views/shared/_locale_switcher.html.erb index c62d2a53d..294badcb8 100644 --- a/app/views/shared/_locale_switcher.html.erb +++ b/app/views/shared/_locale_switcher.html.erb @@ -1,5 +1,5 @@ <% if I18n.available_locales.size > 1 %> -
+
diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 650e44785..a58e5e6ba 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -226,7 +226,6 @@ en: header: administration_menu: Menu administration: Administration - available_locales: Available languages collaborative_legislation: Collaborative legislation debates: Debates locale: "Language:" diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index 7c59338f9..81a903d8c 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -226,7 +226,6 @@ es: header: administration_menu: Menú administration: Administración - available_locales: Idiomas disponibles collaborative_legislation: Legislación colaborativa debates: Debates locale: "Idioma:" 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 04/13] 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 '
    ' + "\n" + + '
  • Home
    • ' + "\n" + + '
  • Info
    • ' + "\n
" + 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 '
    ' + "\n" + + '
  • Home
    • ' + "\n" + + '
  • Info
    • ' + "\n
" + 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 From 7abca09e038a3aa653e9c39962c206bdae0fcba6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 26 Jun 2021 23:14:45 +0200 Subject: [PATCH 05/13] Extract methods in link list component Since we're simplifying the main method, we can use a view file instead of the `call` method. This way we make the code more consistent with the rest of our components, since we always use a separate file. Doing so generates an extra newline at the end of the generated HTML, so we need to change a couple of tests a little bit. --- .../shared/link_list_component.html.erb | 1 + app/components/shared/link_list_component.rb | 16 ++++++++++------ .../shared/link_list_component_spec.rb | 4 ++-- 3 files changed, 13 insertions(+), 8 deletions(-) create mode 100644 app/components/shared/link_list_component.html.erb diff --git a/app/components/shared/link_list_component.html.erb b/app/components/shared/link_list_component.html.erb new file mode 100644 index 000000000..86239013b --- /dev/null +++ b/app/components/shared/link_list_component.html.erb @@ -0,0 +1 @@ +<%= tag.ul(options) { safe_join(list_items, "\n") } %> diff --git a/app/components/shared/link_list_component.rb b/app/components/shared/link_list_component.rb index 840fafe47..683bc80db 100644 --- a/app/components/shared/link_list_component.rb +++ b/app/components/shared/link_list_component.rb @@ -7,12 +7,17 @@ class Shared::LinkListComponent < ApplicationComponent end def render? - links.select(&:present?).any? + present_links.any? end - def call - tag.ul(options) do - safe_join(links.select(&:present?).map do |text, url, current = false, **link_options| + private + + def present_links + links.select(&:present?) + end + + def list_items + present_links.map do |text, url, current = false, **link_options| tag.li(({ "aria-current": true } if current)) do if url link_to text, url, link_options @@ -20,7 +25,6 @@ class Shared::LinkListComponent < ApplicationComponent text end end - end, "\n") + end end - end end diff --git a/spec/components/shared/link_list_component_spec.rb b/spec/components/shared/link_list_component_spec.rb index 2db5e3012..7f951fb30 100644 --- a/spec/components/shared/link_list_component_spec.rb +++ b/spec/components/shared/link_list_component_spec.rb @@ -21,7 +21,7 @@ describe Shared::LinkListComponent, type: :component do expect(list).to eq '
    ' + "\n" + '
  • Home
    • ' + "\n" + - '
  • Info
    • ' + "\n
" + '
  • Info
    • ' + "\n\n" end it "accepts anchor tags" do @@ -32,7 +32,7 @@ describe Shared::LinkListComponent, type: :component do expect(list).to eq '
      ' + "\n" + '
    • Home
      • ' + "\n" + - '
    • Info
      • ' + "\n
    " + '
  • Info
    • ' + "\n\n" end it "accepts options for links" do From ff0f2215ea1b5a7b506bb570df5ae9518075456b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 28 Jun 2021 14:36:19 +0200 Subject: [PATCH 06/13] Extract component for locale switcher Note that in order to simplify the component tests (which for some reason seem to be whitespace-sensitive), we have to omit whitespace characters inside the `
    diff --git a/app/components/layout/locale_switcher_component.rb b/app/components/layout/locale_switcher_component.rb index e0182c2b4..1983e176b 100644 --- a/app/components/layout/locale_switcher_component.rb +++ b/app/components/layout/locale_switcher_component.rb @@ -10,4 +10,10 @@ class Layout::LocaleSwitcherComponent < ApplicationComponent def locales I18n.available_locales end + + def language_options + locales.map do |locale| + [name_for_locale(locale), current_path_with_query_params(locale: locale), lang: locale] + end + end end From 1f10afac640421d2864089056c2790af984f481a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 30 Jun 2021 20:17:28 +0200 Subject: [PATCH 12/13] Simplify language selection with a few languages As mentioned in commit 5214d89c8, using the `change` event of a `select` field to automatically change location is really annoying for keyboard users, since the event will trigger when pressing the down key to navigate through the options or when typing a key to start searching for an option. This might cause a lot of frustration. Most multilanguage CONSUL sites enable between 2 and 4 languages. In these cases, it's easier to just display the list of languages to simplify the selection. This way in this situation we also make it clear which languages are available. If we use a ` - <%= options_for_select(language_options, current_path_with_query_params(locale: I18n.locale)) %> - - + <% if many_locales? %> +
    + + +
    + <% else %> +

    <%= label %>

    + + <%= link_list(*language_links, "aria-labelledby": label_id) %> + <% end %> diff --git a/app/components/layout/locale_switcher_component.rb b/app/components/layout/locale_switcher_component.rb index 1983e176b..4e7408adf 100644 --- a/app/components/layout/locale_switcher_component.rb +++ b/app/components/layout/locale_switcher_component.rb @@ -1,5 +1,5 @@ class Layout::LocaleSwitcherComponent < ApplicationComponent - delegate :name_for_locale, :current_path_with_query_params, to: :helpers + delegate :name_for_locale, :link_list, :current_path_with_query_params, to: :helpers def render? locales.size > 1 @@ -7,13 +7,36 @@ class Layout::LocaleSwitcherComponent < ApplicationComponent private + def many_locales? + locales.size > 4 + end + def locales I18n.available_locales end - def language_options + def label + t("layouts.header.locale") + end + + def label_id + "locale_switcher_label" + end + + def language_links locales.map do |locale| - [name_for_locale(locale), current_path_with_query_params(locale: locale), lang: locale] + [ + name_for_locale(locale), + current_path_with_query_params(locale: locale), + locale == I18n.locale, + lang: locale + ] + end + end + + def language_options + language_links.map do |text, path, _, options| + [text, path, options] end end end diff --git a/spec/components/layout/locale_switcher_component_spec.rb b/spec/components/layout/locale_switcher_component_spec.rb index c4ae8b1be..5d1462e49 100644 --- a/spec/components/layout/locale_switcher_component_spec.rb +++ b/spec/components/layout/locale_switcher_component_spec.rb @@ -28,6 +28,7 @@ describe Layout::LocaleSwitcherComponent, type: :component do expect(page).to have_css "form" expect(page).to have_select "Language:", options: %w[Deutsch English Español Français Nederlands] + expect(page).not_to have_css "ul" end it "selects the current locale" do @@ -53,4 +54,27 @@ describe Layout::LocaleSwitcherComponent, type: :component do end end end + + context "with a few languages" do + before do + allow(I18n).to receive(:available_locales).and_return(%i[en es fr]) + end + + it "renders a list of links" do + render_inline component + + expect(page).to have_css "ul" + expect(page).to have_link "English", href: "/?locale=en" + expect(page).to have_link "Español", href: "/?locale=es" + expect(page).to have_link "Français", href: "/?locale=fr" + expect(page).not_to have_css "form" + end + + it "marks the current locale" do + render_inline component + + expect(page).to have_css "[aria-current]", count: 1 + expect(page).to have_css "[aria-current]", exact_text: "English" + end + end end From aa46444edad44cfab3ce79436814354cfb9ef4ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 5 Jul 2021 15:35:06 +0200 Subject: [PATCH 13/13] Make icon to select language more prominent It was barely visible against a dark background. --- app/assets/stylesheets/layout/locale_switcher.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/layout/locale_switcher.scss b/app/assets/stylesheets/layout/locale_switcher.scss index c3f7c2be3..1625043bb 100644 --- a/app/assets/stylesheets/layout/locale_switcher.scss +++ b/app/assets/stylesheets/layout/locale_switcher.scss @@ -10,7 +10,7 @@ position: relative; &::after { - color: #808080; + color: $light-gray; font-size: $small-font-size; margin-right: 0; pointer-events: none;