From 6ecb6757a66ec66962babf2c9f7cfb11173250f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 6 Feb 2021 20:03:37 +0100 Subject: [PATCH 1/2] Use real check boxes to select goals We were using list items with the checkbox role. This is probably confusing since list items have a listitem role, and so we were basically using a list with no listitem. We could add a `` tag inside the list item, but using real checkboxes is probably easier. We're also adding a test to verify the checkboxes native behavior is compatible with our code. Note we're using the "enter" key to toggle the "checked" status of the SDG. This is probably not intuitive for screen reader users who might try to submit the form using the "enter" key after selecting a goal. However, the alternative would be even less intuitive for sighted keyboard users, since for them these icons look like links or buttons and they would accidentally submit the form when trying to select a goal. Since we haven't come up with a better interface yet, for now we're following the principle of least damage; we consider submitting the form against a user's will is less annoying than forcing users to move to a different field before being able to submit the form. Also note we can't write `check` in the tests because Capybara will try to click the checkbox, which is hidden by the image in the label. --- .../javascripts/sdg/related_list_selector.js | 23 +++++++-------- app/assets/stylesheets/mixins.scss | 2 +- .../sdg/related_list_selector.scss | 28 ++++++++++++------- .../related_list_selector_component.html.erb | 11 ++++---- .../sdg/related_list_selector_component.rb | 11 +++++--- spec/support/common_actions.rb | 4 ++- spec/system/sdg_management/relations_spec.rb | 25 ++++++++++++++--- 7 files changed, 67 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/sdg/related_list_selector.js b/app/assets/javascripts/sdg/related_list_selector.js index 8836acd77..f36c1f7b9 100644 --- a/app/assets/javascripts/sdg/related_list_selector.js +++ b/app/assets/javascripts/sdg/related_list_selector.js @@ -30,11 +30,11 @@ var keep_goal = $(amsify_suggestags.selector).val().split(",").some(function(selected_value) { return App.SDGRelatedListSelector.goal_code(value) === App.SDGRelatedListSelector.goal_code(selected_value); }); - App.SDGRelatedListSelector.goal_element(value).attr("aria-checked", keep_goal); + App.SDGRelatedListSelector.goal_element(value).prop("checked", keep_goal); App.SDGRelatedListSelector.manage_remove_help(amsify_suggestags, value); }, afterAdd: function(value) { - App.SDGRelatedListSelector.goal_element(value).attr("aria-checked", true); + App.SDGRelatedListSelector.goal_element(value).prop("checked", true); App.SDGRelatedListSelector.manage_add_help(amsify_suggestags, value); }, keepLastOnHoverTag: false, @@ -48,23 +48,24 @@ } }, manage_icons: function(amsify_suggestags) { - $("[role='checkbox']").on("click keydown", function(event) { + $(".sdg-related-list-selector .goals input").on("change", function() { var goal_id = this.dataset.code; - if (event.type === "click" || (event.type === "keydown" && [13, 32].indexOf(event.keyCode) >= 0)) { - if (amsify_suggestags.isPresent(goal_id)) { - amsify_suggestags.removeTag(goal_id, false); - } else { - amsify_suggestags.addTag(goal_id, false); - } - + if (amsify_suggestags.isPresent(goal_id)) { + amsify_suggestags.removeTag(goal_id, false); + } else { + amsify_suggestags.addTag(goal_id, false); + } + }).on("keydown", function(event) { + if (event.keyCode === 13) { + $(this).trigger("click"); event.preventDefault(); event.stopPropagation(); } }); }, goal_element: function(value) { - return $("li[data-code=" + App.SDGRelatedListSelector.goal_code(value) + "]"); + return $(".sdg-related-list-selector .goals [data-code=" + App.SDGRelatedListSelector.goal_code(value) + "]"); }, goal_code: function(value) { return value.toString().split(".")[0]; diff --git a/app/assets/stylesheets/mixins.scss b/app/assets/stylesheets/mixins.scss index 0974541c1..fe55cf996 100644 --- a/app/assets/stylesheets/mixins.scss +++ b/app/assets/stylesheets/mixins.scss @@ -240,7 +240,7 @@ width: calc(100% + #{$spacing}); width: calc(100% + #{$max-spacing}); - li { + > * { margin-bottom: 0; margin-left: $spacing; margin-left: $max-spacing; diff --git a/app/assets/stylesheets/sdg/related_list_selector.scss b/app/assets/stylesheets/sdg/related_list_selector.scss index a2de54bd5..684ef2022 100644 --- a/app/assets/stylesheets/sdg/related_list_selector.scss +++ b/app/assets/stylesheets/sdg/related_list_selector.scss @@ -12,22 +12,18 @@ } } - label + ul { + .goals { $spacing: 0.5%; @include sdg-goal-list($spacing); - li { + legend { + @include element-invisible; + } + + label { min-width: $sdg-icon-min-width; width: calc(100% / 17 - #{$spacing}); - &[aria-checked=true] img { - opacity: 0.15; - } - - &:focus { - outline: $outline-focus; - } - &:hover { cursor: pointer; } @@ -36,6 +32,18 @@ width: 100%; } } + + input { + @include element-invisible; + + &:focus + label { + outline: $outline-focus; + } + + &:checked + label img { + opacity: 0.15; + } + } } .input-section { diff --git a/app/components/sdg/related_list_selector_component.html.erb b/app/components/sdg/related_list_selector_component.html.erb index 3697bfa29..24e408e44 100644 --- a/app/components/sdg/related_list_selector_component.html.erb +++ b/app/components/sdg/related_list_selector_component.html.erb @@ -2,13 +2,12 @@
<%= f.label :related_sdg_list %> -
    "> - <% goals.each do |goal| %> -
  • - <%= render SDG::Goals::IconComponent.new(goal) %> -
    • +
    + <%= t("sdg.related_list_selector.goal_list") %> + <%= f.collection_check_boxes(:sdg_goal_ids, goals, :id, :code) do |checkbox_form| %> + <%= goal_field(checkbox_form) %> <% end %> -
+ <%= f.text_field :related_sdg_list, class: "input", diff --git a/app/components/sdg/related_list_selector_component.rb b/app/components/sdg/related_list_selector_component.rb index 4f32adf5a..a17ca1f91 100644 --- a/app/components/sdg/related_list_selector_component.rb +++ b/app/components/sdg/related_list_selector_component.rb @@ -5,10 +5,6 @@ class SDG::RelatedListSelectorComponent < ApplicationComponent @f = form end - def checked?(code) - f.object.sdg_goals.find_by(code: code).present? - end - def sdg_related_suggestions goals_and_targets.map { |goal_or_target| suggestion_tag_for(goal_or_target) } end @@ -39,6 +35,13 @@ class SDG::RelatedListSelectorComponent < ApplicationComponent SDG::Goal.order(:code) end + def goal_field(checkbox_form) + goal = checkbox_form.object + + checkbox_form.check_box(data: { code: goal.code }) + + checkbox_form.label { render(SDG::Goals::IconComponent.new(goal)) } + end + def text_for(goal_or_target) if goal_or_target.class.name == "SDG::Goal" t("sdg.related_list_selector.goal_identifier", code: goal_or_target.code) diff --git a/spec/support/common_actions.rb b/spec/support/common_actions.rb index 9c9f267cc..6702b6609 100644 --- a/spec/support/common_actions.rb +++ b/spec/support/common_actions.rb @@ -45,7 +45,9 @@ module CommonActions end def click_sdg_goal(code) - find("li[data-code='#{code}']").click + within(".sdg-related-list-selector .goals") do + find("[data-code='#{code}'] + label").click + end end def remove_sdg_goal_or_target_tag(code) diff --git a/spec/system/sdg_management/relations_spec.rb b/spec/system/sdg_management/relations_spec.rb index be5bff406..43116cc13 100644 --- a/spec/system/sdg_management/relations_spec.rb +++ b/spec/system/sdg_management/relations_spec.rb @@ -368,7 +368,7 @@ describe "SDG Relations", :js do visit sdg_management_edit_legislation_process_path(process) click_sdg_goal(1) - expect(find("li[data-code='1']")["aria-checked"]).to eq "true" + expect(find("input[data-code='1']")).to be_checked end scenario "when remove a last tag related to a Goal, the icon will not be checked" do @@ -380,15 +380,32 @@ describe "SDG Relations", :js do visit sdg_management_edit_legislation_process_path(process) remove_sdg_goal_or_target_tag(1) - expect(find("li[data-code='1']")["aria-checked"]).to eq "true" + expect(find("input[data-code='1']")).to be_checked remove_sdg_goal_or_target_tag(1.1) - expect(find("li[data-code='1']")["aria-checked"]).to eq "true" + expect(find("input[data-code='1']")).to be_checked remove_sdg_goal_or_target_tag("1.1.1") - expect(find("li[data-code='1']")["aria-checked"]).to eq "false" + expect(find("input[data-code='1']")).not_to be_checked + end + + context "when we have a Goal and a related Target selected" do + scenario "we can remove and add same Goal always keeping the icon as checked" do + process = create(:legislation_process, title: "SDG process") + process.sdg_goals = [SDG::Goal[1]] + process.sdg_targets = [SDG::Target[1.1]] + + visit sdg_management_edit_legislation_process_path(process) + click_sdg_goal(1) + + expect(find("input[data-code='1']")).to be_checked + + click_sdg_goal(1) + + expect(find("input[data-code='1']")).to be_checked + end end end From ab2b3a26716d4ce5987310205dd2b5e1f1290912 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 7 Feb 2021 15:19:49 +0100 Subject: [PATCH 2/2] Improve help messages in SDG selector Hopefully now it's a bit more obvious that SDGs can be selected by clicking on them and that the field to select goals and the field to select targets/goals are related (since they're now part of the same fieldset). --- .../stylesheets/sdg/related_list_selector.scss | 12 +++++++++++- .../sdg/related_list_selector_component.html.erb | 15 +++++++-------- .../sdg/related_list_selector_component.rb | 4 ++++ config/locales/en/activerecord.yml | 1 - config/locales/en/sdg.yml | 3 ++- config/locales/es/activerecord.yml | 1 - config/locales/es/sdg.yml | 3 ++- spec/system/sdg_management/relations_spec.rb | 8 ++++---- 8 files changed, 30 insertions(+), 17 deletions(-) diff --git a/app/assets/stylesheets/sdg/related_list_selector.scss b/app/assets/stylesheets/sdg/related_list_selector.scss index 684ef2022..9e4f8c0ad 100644 --- a/app/assets/stylesheets/sdg/related_list_selector.scss +++ b/app/assets/stylesheets/sdg/related_list_selector.scss @@ -2,6 +2,10 @@ @include grid-column-gutter; clear: both; + > legend { + margin-bottom: 0; + } + .amsify-suggestags-area .amsify-select-tag { color: $white; @@ -17,7 +21,8 @@ @include sdg-goal-list($spacing); legend { - @include element-invisible; + font-weight: normal; + font-style: italic; } label { @@ -46,6 +51,11 @@ } } + label { + font-weight: normal; + font-style: italic; + } + .input-section { margin-bottom: 2 * $line-height / 3; } diff --git a/app/components/sdg/related_list_selector_component.html.erb b/app/components/sdg/related_list_selector_component.html.erb index 24e408e44..a1bbca60e 100644 --- a/app/components/sdg/related_list_selector_component.html.erb +++ b/app/components/sdg/related_list_selector_component.html.erb @@ -1,9 +1,9 @@ -
-
- <%= f.label :related_sdg_list %> +
+ <%= t("sdg.related_list_selector.title") %> +
- <%= t("sdg.related_list_selector.goal_list") %> + <%= t("sdg.related_list_selector.goal_list", record: relatable_name) %> <%= f.collection_check_boxes(:sdg_goal_ids, goals, :id, :code) do |checkbox_form| %> <%= goal_field(checkbox_form) %> <% end %> @@ -11,15 +11,14 @@ <%= f.text_field :related_sdg_list, class: "input", - label: false, placeholder: t("sdg.related_list_selector.placeholder"), - hint: t("sdg.related_list_selector.hint"), + label: t("sdg.related_list_selector.hint"), data: { "suggestions-list": sdg_related_suggestions, "remove-tag-text": t("sdg.related_list_selector.remove_tag") } %>
-

<%= t("sdg.related_list_selector.help.title", record: f.object.model_name.human) %>

+

<%= t("sdg.related_list_selector.help.title", record: relatable_name) %>

    -
    + diff --git a/app/components/sdg/related_list_selector_component.rb b/app/components/sdg/related_list_selector_component.rb index a17ca1f91..2dedae2d2 100644 --- a/app/components/sdg/related_list_selector_component.rb +++ b/app/components/sdg/related_list_selector_component.rb @@ -49,4 +49,8 @@ class SDG::RelatedListSelectorComponent < ApplicationComponent goal_or_target.code end end + + def relatable_name + f.object.model_name.human.downcase + end end diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index 825386440..ce8db966a 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -2,7 +2,6 @@ en: attributes: geozone_id: "Scope of operation" results_enabled: "Show results" - related_sdg_list: "Sustainable Development Goals and Targets" stats_enabled: "Show stats" advanced_stats_enabled: "Show advanced stats" name: Name diff --git a/config/locales/en/sdg.yml b/config/locales/en/sdg.yml index 9ce18e282..0eb3a869c 100644 --- a/config/locales/en/sdg.yml +++ b/config/locales/en/sdg.yml @@ -455,12 +455,13 @@ en: other: "%{count} more goals" related_list_selector: goal_identifier: "SDG%{code}" - goal_list: "Goal list" + goal_list: "You can choose one or several SDGs aligned with your %{record}" help: title: "Which SDGs and targets are aligned with my %{record}?" hint: "You can introduce the code of a specific goal/target or a text to find one" placeholder: "Write a goal or target code or description" remove_tag: "Remove" + title: "Sustainable Development Goals and Targets" targets: filter: link: "See all %{resources} related to target %{code}" diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index b98bafcb2..86f05b5ff 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -2,7 +2,6 @@ es: attributes: geozone_id: "Ámbito de actuación" results_enabled: "Mostrar resultados" - related_sdg_list: "Objetivos de Desarrollo Sostenible y Metas" stats_enabled: "Mostrar estadísticas" advanced_stats_enabled: "Mostrar estadísticas avanzadas" name: Nombre diff --git a/config/locales/es/sdg.yml b/config/locales/es/sdg.yml index 7c864ce93..a3f4f9561 100644 --- a/config/locales/es/sdg.yml +++ b/config/locales/es/sdg.yml @@ -455,12 +455,13 @@ es: other: "%{count} objetivos más" related_list_selector: goal_identifier: "ODS%{code}" - goal_list: "Listado de objetivos" + goal_list: "Puedes seleccionar uno o varios ODS con los que se alinea tu %{record}" help: title: "¿Qué ODS y metas se alinean con mi %{record}?" hint: "Puedes introducir el código de un objetivo/meta específico o un texto para encontrar uno" placeholder: "Escribe las etiquetas que desees" remove_tag: "Eliminar" + title: "Objetivos de Desarrollo Sostenible y Metas" targets: filter: link: "Ver %{resources} de la meta %{code}" diff --git a/spec/system/sdg_management/relations_spec.rb b/spec/system/sdg_management/relations_spec.rb index 43116cc13..1f647ef30 100644 --- a/spec/system/sdg_management/relations_spec.rb +++ b/spec/system/sdg_management/relations_spec.rb @@ -299,17 +299,17 @@ describe "SDG Relations", :js do create(:sdg_local_target, code: "1.1.1") visit sdg_management_edit_legislation_process_path(process) - fill_in "Sustainable Development Goals and Targets", with: "3" + fill_in "You can introduce the code of a specific goal/target or a text to find one", with: "3" within(".amsify-list") { find(:css, "[data-val='3']").click } within(".amsify-suggestags-input-area") { expect(page).to have_content "SDG3" } - fill_in "Sustainable Development Goals and Targets", with: "1.1" + fill_in "You can introduce the code of a specific goal/target or a text to find one", with: "1.1" within(".amsify-list") { find(:css, "[data-val='1.1']").click } within(".amsify-suggestags-input-area") { expect(page).to have_content "1.1" } - fill_in "Sustainable Development Goals and Targets", with: "1.1.1" + fill_in "You can introduce the code of a specific goal/target or a text to find one", with: "1.1.1" within(".amsify-list") { find(:css, "[data-val='1.1.1']").click } within(".amsify-suggestags-input-area") { expect(page).to have_content "1.1.1" } @@ -327,8 +327,8 @@ describe "SDG Relations", :js do process = create(:legislation_process, title: "SDG process") visit sdg_management_edit_legislation_process_path(process) + fill_in "You can introduce the code of a specific goal/target or a text to find one", with: "tag nonexistent," - fill_in "Sustainable Development Goals and Targets", with: "tag nonexistent," within(".amsify-suggestags-input-area") { expect(page).not_to have_content "tag nonexistent" } end