From 45a80af793fe7d4641621437f9025105bd5a1292 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Mon, 27 Jul 2020 11:29:37 +0200 Subject: [PATCH 1/7] Do not remove click event definition before defining it Use delegated handlers instead so there is not risk to run method multiple times. --- app/assets/javascripts/forms.js | 4 ++-- app/assets/javascripts/legislation_annotatable.js | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/forms.js b/app/assets/javascripts/forms.js index 28a92bc6b..822b2391b 100644 --- a/app/assets/javascripts/forms.js +++ b/app/assets/javascripts/forms.js @@ -9,13 +9,13 @@ }); }, submitOnChange: function() { - $(".js-submit-on-change").off("change").on("change", function() { + $("body").on("change", ".js-submit-on-change", function() { $(this).closest("form").submit(); return false; }); }, toggleLink: function() { - $(".js-toggle-link").off("click").on("click", function() { + $("body").on("click", ".js-toggle-link", function() { var toggle_txt; $($(this).data("toggle-selector")).toggle("down"); if ($(this).data("toggle-text") !== undefined) { diff --git a/app/assets/javascripts/legislation_annotatable.js b/app/assets/javascripts/legislation_annotatable.js index 982dfaae6..9b1125722 100644 --- a/app/assets/javascripts/legislation_annotatable.js +++ b/app/assets/javascripts/legislation_annotatable.js @@ -69,14 +69,14 @@ $("#comments-box").html(""); App.LegislationAllegations.show_comments(); $("#comments-box").show(); - $.event.trigger({ + $("body").trigger({ type: "renderLegislationAnnotation", annotation_id: target.data("annotation-id"), annotation_url: target.closest(".legislation-annotatable").data("legislation-annotatable-base-url"), offset: target.offset().top }); parents_ids.each(function(i, pid) { - $.event.trigger({ + $("body").trigger({ type: "renderLegislationAnnotation", annotation_id: pid, annotation_url: target.closest(".legislation-annotatable").data("legislation-annotatable-base-url") @@ -144,7 +144,7 @@ $("html,body").animate({ scrollTop: el.offset().top }); - $.event.trigger({ + $("body").trigger({ type: "renderLegislationAnnotation", annotation_id: ann_id, annotation_url: el.closest(".legislation-annotatable").data("legislation-annotatable-base-url"), @@ -188,9 +188,9 @@ }, initialize: function() { var current_user_id; - $(document).off("renderLegislationAnnotation").on("renderLegislationAnnotation", App.LegislationAnnotatable.renderAnnotationComments); - $(document).off("click", "[data-annotation-id]").on("click", "[data-annotation-id]", App.LegislationAnnotatable.onClick); - $(document).off("click", "[data-cancel-annotation]").on("click", "[data-cancel-annotation]", function(e) { + $("body").on("renderLegislationAnnotation", App.LegislationAnnotatable.renderAnnotationComments); + $("body").on("click", "[data-annotation-id]", App.LegislationAnnotatable.onClick); + $("body").on("click", "[data-cancel-annotation]", function(e) { e.preventDefault(); $("#comments-box").html(""); $("#comments-box").hide(); From 149ce945c040f4f5562f4f3cda7068b97a1e398a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 10 Jul 2020 18:12:04 +0200 Subject: [PATCH 2/7] Simplify initialization code for tags links By using an event delegation handler on document body there is no need to check if element was already initialized (idempotency) anymore. --- app/assets/javascripts/tags.js | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/tags.js b/app/assets/javascripts/tags.js index 312ec892e..5fd318aeb 100644 --- a/app/assets/javascripts/tags.js +++ b/app/assets/javascripts/tags.js @@ -4,21 +4,17 @@ initialize: function() { var $tag_input; $tag_input = $("input.js-tag-list"); - $("body .js-add-tag-link").each(function() { - if ($(this).data("initialized") !== "yes") { - $(this).on("click", function() { - var current_tags, name; - name = "\"" + ($(this).text()) + "\""; - current_tags = $tag_input.val().split(",").filter(Boolean); - if (current_tags.indexOf(name) >= 0) { - current_tags.splice(current_tags.indexOf(name), 1); - } else { - current_tags.push(name); - } - $tag_input.val(current_tags.join(",")); - return false; - }).data("initialized", "yes"); + $("body").on("click", ".js-add-tag-link", function() { + var current_tags, name; + name = "\"" + ($(this).text()) + "\""; + current_tags = $tag_input.val().split(",").filter(Boolean); + if (current_tags.indexOf(name) >= 0) { + current_tags.splice(current_tags.indexOf(name), 1); + } else { + current_tags.push(name); } + $tag_input.val(current_tags.join(",")); + return false; }); } }; From cda545f2fc7964da46d484f2d622613ebd1cb98e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 24 Jul 2020 13:09:29 +0200 Subject: [PATCH 3/7] Remove uneeded lines There is no elements with class `js-participation-allowed` within all application code. --- app/assets/javascripts/allow_participation.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/assets/javascripts/allow_participation.js b/app/assets/javascripts/allow_participation.js index 0b18fb78c..b6223a47c 100644 --- a/app/assets/javascripts/allow_participation.js +++ b/app/assets/javascripts/allow_participation.js @@ -5,11 +5,9 @@ $(document).on({ "mouseenter focus": function() { $(this).find(".js-participation-not-allowed").show(); - $(this).find(".js-participation-allowed").hide(); }, mouseleave: function() { $(this).find(".js-participation-not-allowed").hide(); - $(this).find(".js-participation-allowed").show(); } }, ".js-participation"); } From 99f8bb44911647cce645be66875b0e71ddc41a03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Tue, 28 Jul 2020 11:15:09 +0200 Subject: [PATCH 4/7] Fix collission with `sortable.js` script TableSortable and Sortable javascripts were using the same CSS class name to define completely different and separated behaviours causing unexpected errors. Now `sortable.js` script will use `.sortable` class and `table_sortable.js` will use `.table-sortable` instead. --- app/assets/javascripts/table_sortable.js | 2 +- app/views/admin/stats/polls.html.erb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/table_sortable.js b/app/assets/javascripts/table_sortable.js index f5aa31b74..282f5d42b 100644 --- a/app/assets/javascripts/table_sortable.js +++ b/app/assets/javascripts/table_sortable.js @@ -17,7 +17,7 @@ }; }, initialize: function() { - $("table.sortable th").on("click", function() { + $(".table-sortable th").on("click", function() { var rows, table; table = $(this).parents("table").eq(0); rows = table.find("tbody tr").toArray().sort(App.TableSortable.comparer($(this).index())); diff --git a/app/views/admin/stats/polls.html.erb b/app/views/admin/stats/polls.html.erb index d40e4280b..178d06bcb 100644 --- a/app/views/admin/stats/polls.html.erb +++ b/app/views/admin/stats/polls.html.erb @@ -25,7 +25,7 @@

<%= t("admin.stats.polls.all") %>

- +
@@ -52,7 +52,7 @@

<%= t("admin.stats.polls.poll_questions", poll: poll.name) %>

-
<%= t("admin.stats.polls.table.poll_name") %>
+
From fde6fb4d97302bce51964fd58d0682f4cfded287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Wed, 29 Jul 2020 14:44:05 +0200 Subject: [PATCH 5/7] Initialize only visible maps when page is loaded Its known that initializing a map when it is inside a hidden element wont work when hidden element is shown, so its makes sense to avoid initialization of hidden maps. When a map lives within a hidden layer we need to initialize the map after the event of showing that hidden layer, in our case when admin settings tab is shown. --- app/assets/javascripts/map.js | 2 +- spec/system/admin/settings_spec.rb | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index 2154d98cb..d160d3a92 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -2,7 +2,7 @@ "use strict"; App.Map = { initialize: function() { - $("*[data-map]").each(function() { + $("*[data-map]:visible").each(function() { App.Map.initializeMap(this); }); $(".js-toggle-map").on({ diff --git a/spec/system/admin/settings_spec.rb b/spec/system/admin/settings_spec.rb index b9d0eba95..d9f418d05 100644 --- a/spec/system/admin/settings_spec.rb +++ b/spec/system/admin/settings_spec.rb @@ -30,6 +30,30 @@ describe "Admin settings" do expect(page).to have_content "Value updated" end + describe "Map settings initialization", :js do + before do + Setting["feature.map"] = true + end + + scenario "When `Map settings` tab content is hidden map should not be initialized" do + admin = create(:administrator).user + login_as(admin) + visit admin_settings_path + + expect(page).not_to have_css("#admin-map.leaflet-container", visible: false) + end + + scenario "When `Map settings` tab content is shown map should be initialized" do + admin = create(:administrator).user + login_as(admin) + visit admin_settings_path + + find("#map-tab").click + + expect(page).to have_css("#admin-map.leaflet-container", visible: true) + end + end + describe "Update map" do scenario "Should not be able when map feature deactivated" do Setting["feature.map"] = false From 983bf49b3802e1aae539591264dc07dde3d0dfbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 3 Aug 2020 12:17:25 +0200 Subject: [PATCH 6/7] Simplify code related to Foundation's sticky In the past we had huge problems trying to make it work with Turbolinks. However, after updating foundation-rails in commit 58071fd6, these hacks aren't necessary anymore. We're adding a test for the scenario of visiting a page using Turbolinks, which was missing, so we're sure we aren't breaking anything. Note the sticky will still not work after using the browser back button. We haven't been able to make it work with turbolinks-classic; we'll fix this issue when upgrading turbolinks. --- app/assets/javascripts/foundation_extras.js | 21 --------------------- spec/system/proposals_spec.rb | 17 +++++++++++++++-- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/app/assets/javascripts/foundation_extras.js b/app/assets/javascripts/foundation_extras.js index c6e344684..de709505d 100644 --- a/app/assets/javascripts/foundation_extras.js +++ b/app/assets/javascripts/foundation_extras.js @@ -1,29 +1,8 @@ (function() { "use strict"; App.FoundationExtras = { - clearSticky: function() { - if ($("[data-sticky]").length) { - $("[data-sticky]").foundation("destroy"); - } - }, - mobile_ui_init: function() { - $(window).trigger("load.zf.sticky"); - }, - desktop_ui_init: function() { - $(window).trigger("init.zf.sticky"); - }, initialize: function() { $(document).foundation(); - $(window).trigger("resize"); - $(document).on("page:before-unload", this.clearSticky); - window.addEventListener("popstate", this.clearSticky, false); - $(function() { - if ($(window).width() < 620) { - App.FoundationExtras.mobile_ui_init(); - } else { - App.FoundationExtras.desktop_ui_init(); - } - }); } }; }).call(this); diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index 6374057cf..986fb7daf 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -200,7 +200,7 @@ describe "Proposals" do end end - context "Show on mobile screens" do + describe "Show sticky support button on mobile screens", :js do let!(:window_size) { Capybara.current_window.size } before do @@ -211,7 +211,7 @@ describe "Proposals" do Capybara.current_window.resize_to(*window_size) end - scenario "Show support button sticky at bottom", :js do + scenario "On a first visit" do proposal = create(:proposal) visit proposal_path(proposal) @@ -220,6 +220,19 @@ describe "Proposals" do expect(page).not_to have_css(".is-anchored") end end + + scenario "After visiting another page" do + proposal = create(:proposal) + + visit proposal_path(proposal) + click_link "Go back" + click_link proposal.title + + within("#proposal_sticky") do + expect(page).to have_css(".is-stuck") + expect(page).not_to have_css(".is-anchored") + end + end end context "Embedded video" do From 017eeda3d453a9b6c1ea45157ac4c3453e5ebef6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 4 Aug 2020 18:56:01 +0200 Subject: [PATCH 7/7] Remove redundant call to $(document).ready() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `$()` function is a shortcut for `$(document).ready()`, and we were attaching events to `$(document).ready()` inside the `$()` function. In a similar way, we were handling the `page:load` and `ajax:complete` events inside the `$()` function. But when those events trigger, the DOM is already ready. Besides, we don't have to wait for the DOM to be ready before attaching events to the `document` element. Quoting jQuery's `.on()` documentation: > The document element is available in the head of the document before > loading any other HTML, so it is safe to attach events there without > waiting for the document to be ready. Co-Authored-By: Senén Rodero Rodríguez --- app/assets/javascripts/application.js | 5 ++--- app/assets/javascripts/stat_graphs.js | 10 +++------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index cfdf34113..d3c84a255 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -170,7 +170,6 @@ $(function() { "use strict"; Turbolinks.enableProgressBar(); - - $(document).ready(initialize_modules); - $(document).on("page:load", initialize_modules); }); +$(document).ready(initialize_modules); +$(document).on("page:load", initialize_modules); diff --git a/app/assets/javascripts/stat_graphs.js b/app/assets/javascripts/stat_graphs.js index d3aa121fa..97d743355 100644 --- a/app/assets/javascripts/stat_graphs.js +++ b/app/assets/javascripts/stat_graphs.js @@ -9,10 +9,6 @@ var initialize_stats_modules = function() { App.Stats.initialize(); }; -$(function() { - "use strict"; - - $(document).ready(initialize_stats_modules); - $(document).on("page:load", initialize_stats_modules); - $(document).on("ajax:complete", initialize_stats_modules); -}); +$(document).ready(initialize_stats_modules); +$(document).on("page:load", initialize_stats_modules); +$(document).on("ajax:complete", initialize_stats_modules);
<%= t("admin.stats.polls.table.question_name") %>