From 7b96180a76ce6a6e0bab91558b542edd1e4d1d6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 13 Sep 2019 02:22:10 +0200 Subject: [PATCH 01/15] Upgrade Turbolinks to version 5.2.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We didn't upgrade Turbolinks when we upgraded to Rails 5 so we didn't upgrade too many things at the same time, and postponed it... until now :). Note upgrading Turbolinks fixes an issue with foundation's sticky when using the browser's back and forward buttons. We're adding tests for these scenarios. Co-authored-by: Senén Rodero Rodríguez --- Gemfile | 2 +- Gemfile.lock | 7 ++-- app/assets/javascripts/application.js | 8 +---- app/assets/javascripts/html_editor.js | 3 +- app/assets/javascripts/stat_graphs.js | 3 +- app/assets/javascripts/watch_form_changes.js | 2 +- .../admin/budget_investments/show.html.erb | 2 +- .../admin/stats/budget_supporting.html.erb | 2 +- app/views/admin/stats/graph.html.erb | 2 +- app/views/budgets/groups/show.html.erb | 2 +- app/views/budgets/show.html.erb | 2 +- app/views/dashboard/progress.html.erb | 2 +- app/views/layouts/_common_head.html.erb | 2 +- app/views/layouts/dashboard.html.erb | 2 +- .../layouts/proposals_dashboard.html.erb | 2 +- spec/system/proposals_spec.rb | 36 +++++++++++++++++++ 16 files changed, 54 insertions(+), 25 deletions(-) diff --git a/Gemfile b/Gemfile index 41af19f4a..d6faebd60 100644 --- a/Gemfile +++ b/Gemfile @@ -53,7 +53,7 @@ gem "sitemap_generator", "~> 6.0.2" gem "social-share-button", "~> 1.1" gem "sprockets", "~> 3.7.2" gem "translator-text", "~> 0.1.0" -gem "turbolinks", "~> 2.5.3" +gem "turbolinks", "~> 5.2.1" gem "turnout", "~> 2.4.0" gem "uglifier", "~> 4.1.2" gem "whenever", "~> 0.10.0", require: false diff --git a/Gemfile.lock b/Gemfile.lock index 20329b10a..bf7e738e5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -563,8 +563,9 @@ GEM translator-text (0.1.0) dry-struct (~> 0.5.0) httparty (~> 0.15) - turbolinks (2.5.4) - coffee-rails + turbolinks (5.2.1) + turbolinks-source (~> 5.2) + turbolinks-source (5.2.0) turnout (2.4.1) i18n (~> 0.7) rack (>= 1.3, < 3) @@ -690,7 +691,7 @@ DEPENDENCIES spring-commands-rspec (~> 1.0.4) sprockets (~> 3.7.2) translator-text (~> 0.1.0) - turbolinks (~> 2.5.3) + turbolinks (~> 5.2.1) turnout (~> 2.4.0) uglifier (~> 4.1.2) web-console (~> 3.3.0) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index d3c84a255..b00cd42cd 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -166,10 +166,4 @@ var initialize_modules = function() { App.BudgetEditAssociations.initialize(); }; -$(function() { - "use strict"; - - Turbolinks.enableProgressBar(); -}); -$(document).ready(initialize_modules); -$(document).on("page:load", initialize_modules); +$(document).on("turbolinks:load", initialize_modules); diff --git a/app/assets/javascripts/html_editor.js b/app/assets/javascripts/html_editor.js index 590a473ae..affdc7034 100644 --- a/app/assets/javascripts/html_editor.js +++ b/app/assets/javascripts/html_editor.js @@ -17,6 +17,5 @@ } }; - $(document).on("page:before-unload", App.HTMLEditor.destroy); - $(document).on("page:restore", App.HTMLEditor.initialize); + $(document).on("turbolinks:before-cache", App.HTMLEditor.destroy); }).call(this); diff --git a/app/assets/javascripts/stat_graphs.js b/app/assets/javascripts/stat_graphs.js index 97d743355..b5aa71898 100644 --- a/app/assets/javascripts/stat_graphs.js +++ b/app/assets/javascripts/stat_graphs.js @@ -9,6 +9,5 @@ var initialize_stats_modules = function() { App.Stats.initialize(); }; -$(document).ready(initialize_stats_modules); -$(document).on("page:load", initialize_stats_modules); +$(document).on("turbolinks:load", initialize_stats_modules); $(document).on("ajax:complete", initialize_stats_modules); diff --git a/app/assets/javascripts/watch_form_changes.js b/app/assets/javascripts/watch_form_changes.js index 104d8d4d0..b666ce7ac 100644 --- a/app/assets/javascripts/watch_form_changes.js +++ b/app/assets/javascripts/watch_form_changes.js @@ -23,7 +23,7 @@ if (App.WatchFormChanges.forms().length === 0 || App.WatchFormChanges.msg() === undefined) { return; } - $(document).off("page:before-change").on("page:before-change", App.WatchFormChanges.checkChanges); + $(document).off("turbolinks:before-visit").on("turbolinks:before-visit", App.WatchFormChanges.checkChanges); App.WatchFormChanges.forms().each(function() { $(this).data("watchChanges", $(this).serialize()); }); diff --git a/app/views/admin/budget_investments/show.html.erb b/app/views/admin/budget_investments/show.html.erb index 375c5e978..3727c5f4b 100644 --- a/app/views/admin/budget_investments/show.html.erb +++ b/app/views/admin/budget_investments/show.html.erb @@ -1,5 +1,5 @@ <%= link_to admin_budget_budget_investments_path(Budget::Investment.filter_params(params).to_h), - class: "back", data: { no_turbolink: true } do %> + class: "back", data: { turbolinks: false } do %> <%= t("shared.back") %> <% end %> diff --git a/app/views/admin/stats/budget_supporting.html.erb b/app/views/admin/stats/budget_supporting.html.erb index 6eb995a15..770029969 100644 --- a/app/views/admin/stats/budget_supporting.html.erb +++ b/app/views/admin/stats/budget_supporting.html.erb @@ -1,5 +1,5 @@ <% content_for :head do %> - <%= javascript_include_tag "stat_graphs", "data-turbolinks-track" => true %> + <%= javascript_include_tag "stat_graphs", "data-turbolinks-track" => "reload" %> <% end %> <%= back_link_to budgets_admin_stats_path %> diff --git a/app/views/admin/stats/graph.html.erb b/app/views/admin/stats/graph.html.erb index 13c473347..0a9828806 100644 --- a/app/views/admin/stats/graph.html.erb +++ b/app/views/admin/stats/graph.html.erb @@ -1,5 +1,5 @@ <% content_for :head do %> - <%= javascript_include_tag "stat_graphs", "data-turbolinks-track" => true %> + <%= javascript_include_tag "stat_graphs", "data-turbolinks-track" => "reload" %> <% end %> <%= render "graph", name: @name, event: @event, count: @count %> diff --git a/app/views/budgets/groups/show.html.erb b/app/views/budgets/groups/show.html.erb index 2af9cb372..5ebb18ace 100644 --- a/app/views/budgets/groups/show.html.erb +++ b/app/views/budgets/groups/show.html.erb @@ -36,7 +36,7 @@ <%= link_to heading.name, budget_investments_path(heading_id: heading.id, filter: @current_filter), - data: { no_turbolink: true } %>
+ data: { turbolinks: false } %>
<% end %> diff --git a/app/views/budgets/show.html.erb b/app/views/budgets/show.html.erb index 3949208be..a35affe18 100644 --- a/app/views/budgets/show.html.erb +++ b/app/views/budgets/show.html.erb @@ -64,7 +64,7 @@ budget_investments_path(@budget, heading_id: group.headings.first.id, filter: @current_filter), - data: { no_turbolink: true } %> + data: { turbolinks: false } %> <% else %> <%= link_to group.name, budget_group_path(@budget, group, diff --git a/app/views/dashboard/progress.html.erb b/app/views/dashboard/progress.html.erb index bfd32c5f3..de9e94be8 100644 --- a/app/views/dashboard/progress.html.erb +++ b/app/views/dashboard/progress.html.erb @@ -29,7 +29,7 @@ class="c3 proposal-graph"> - <%= javascript_include_tag "dashboard_graphs", "data-turbolinks-track" => true %> + <%= javascript_include_tag "dashboard_graphs", "data-turbolinks-track" => "reload" %> <% end %> <%= render "next_goal" %> diff --git a/app/views/layouts/_common_head.html.erb b/app/views/layouts/_common_head.html.erb index 377045e03..f034f7baf 100644 --- a/app/views/layouts/_common_head.html.erb +++ b/app/views/layouts/_common_head.html.erb @@ -3,7 +3,7 @@ <%= content_for?(:title) ? yield(:title) : default_title %> <%= stylesheet_link_tag "application" %> -<%= javascript_include_tag "application", "data-turbolinks-track" => true %> +<%= javascript_include_tag "application", "data-turbolinks-track" => "reload" %> <%= csrf_meta_tags %> <%= favicon_link_tag "favicon.ico" %> <%= render "layouts/disable_animations_in_tests" if Rails.env.test? %> diff --git a/app/views/layouts/dashboard.html.erb b/app/views/layouts/dashboard.html.erb index 898298a50..2b20ca089 100644 --- a/app/views/layouts/dashboard.html.erb +++ b/app/views/layouts/dashboard.html.erb @@ -9,7 +9,7 @@ <%= content_for?(:title) ? yield(:title) : setting["org_name"] %> <%= content_for :canonical %> <%= stylesheet_link_tag "application" %> - <%= javascript_include_tag "application", "data-turbolinks-track" => true %> + <%= javascript_include_tag "application", "data-turbolinks-track" => "reload" %> <%= csrf_meta_tags %> <%= favicon_link_tag "favicon.ico" %> <%= favicon_link_tag image_path_for("apple-touch-icon-200.png"), diff --git a/app/views/layouts/proposals_dashboard.html.erb b/app/views/layouts/proposals_dashboard.html.erb index 3ae31f738..050fde8a6 100644 --- a/app/views/layouts/proposals_dashboard.html.erb +++ b/app/views/layouts/proposals_dashboard.html.erb @@ -9,7 +9,7 @@ <%= content_for?(:title) ? yield(:title) : setting["org_name"] %> <%= content_for :canonical %> <%= stylesheet_link_tag "application" %> - <%= javascript_include_tag "application", "data-turbolinks-track" => true %> + <%= javascript_include_tag "application", "data-turbolinks-track" => "reload" %> <%= csrf_meta_tags %> <%= favicon_link_tag "favicon.ico" %> <%= favicon_link_tag image_path_for("apple-touch-icon-200.png"), diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index 986fb7daf..d0978fa18 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -233,6 +233,42 @@ describe "Proposals" do expect(page).not_to have_css(".is-anchored") end end + + scenario "After using the browser's back button" do + proposal = create(:proposal) + + visit proposal_path(proposal) + click_link "Go back" + + expect(page).to have_link proposal.title + + go_back + + within("#proposal_sticky") do + expect(page).to have_css(".is-stuck") + expect(page).not_to have_css(".is-anchored") + end + end + + scenario "After using the browser's forward button" do + proposal = create(:proposal) + + visit proposals_path + click_link proposal.title + + expect(page).not_to have_link proposal.title + + go_back + + expect(page).to have_link proposal.title + + go_forward + + 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 ae2b4b16c6965308a82515731715e3857e743756 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 13 Sep 2019 17:40:13 +0200 Subject: [PATCH 02/15] Prevent AJAX requests to anchor links Turbolinks 5 doesn't follow the browser's standard behaviour of ignoring links pointing to "#", so we're preventing the turbolinks events in this situation: --- app/assets/javascripts/application.js | 1 + app/assets/javascripts/turbolinks_anchors.js | 15 +++++++++++++++ 2 files changed, 16 insertions(+) create mode 100644 app/assets/javascripts/turbolinks_anchors.js diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index b00cd42cd..4d1eb6aa2 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -42,6 +42,7 @@ //= require jquery-fileupload/basic //= require foundation //= require turbolinks +//= require turbolinks_anchors //= require ckeditor/loader //= require_directory ./ckeditor //= require social-share-button diff --git a/app/assets/javascripts/turbolinks_anchors.js b/app/assets/javascripts/turbolinks_anchors.js new file mode 100644 index 000000000..65923c0fb --- /dev/null +++ b/app/assets/javascripts/turbolinks_anchors.js @@ -0,0 +1,15 @@ +(function() { + "use strict"; + + // Code by Dom Christie: + // https://github.com/turbolinks/turbolinks/issues/75#issuecomment-443256173 + document.addEventListener("turbolinks:click", function(event) { + if (event.target.getAttribute("href").charAt(0) === "#") { + Turbolinks.controller.pushHistoryWithLocationAndRestorationIdentifier( + event.data.url, + Turbolinks.uuid() + ); + event.preventDefault(); + } + }); +}).call(this); From f8c9f09887b533dc55afd7a82ef6790a6e8d3e92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 13 Sep 2019 17:44:12 +0200 Subject: [PATCH 03/15] Prevent default event on JavaScript-only links These links point to "#" and don't do anything without JavaScript activated, and they were causing the browser to scroll to the top of the page. --- app/assets/javascripts/check_all_none.js | 8 ++++++-- app/assets/javascripts/managers.js | 8 ++++++-- app/assets/javascripts/markdown_editor.js | 4 +++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/check_all_none.js b/app/assets/javascripts/check_all_none.js index 8684a7cf7..ac52cdd79 100644 --- a/app/assets/javascripts/check_all_none.js +++ b/app/assets/javascripts/check_all_none.js @@ -2,13 +2,17 @@ "use strict"; App.CheckAllNone = { initialize: function() { - $("[data-check-all]").on("click", function() { + $("[data-check-all]").on("click", function(e) { var target_name; + e.preventDefault(); + e.stopPropagation(); target_name = $(this).data("check-all"); $("[name='" + target_name + "']").prop("checked", true); }); - $("[data-check-none]").on("click", function() { + $("[data-check-none]").on("click", function(e) { var target_name; + e.preventDefault(); + e.stopPropagation(); target_name = $(this).data("check-none"); $("[name='" + target_name + "']").prop("checked", false); }); diff --git a/app/assets/javascripts/managers.js b/app/assets/javascripts/managers.js index 4a87fa7e4..b66914f2a 100644 --- a/app/assets/javascripts/managers.js +++ b/app/assets/javascripts/managers.js @@ -17,10 +17,14 @@ $("#user_password").prop("type", type); }, initialize: function() { - $(".generate-random-value").on("click", function() { + $(".generate-random-value").on("click", function(e) { + e.preventDefault(); + e.stopPropagation(); $("#user_password").val(App.Managers.generatePassword()); }); - $(".show-password").on("click", function() { + $(".show-password").on("click", function(e) { + e.preventDefault(); + e.stopPropagation(); if ($("#user_password").is("input[type='password']")) { App.Managers.togglePassword("text"); } else { diff --git a/app/assets/javascripts/markdown_editor.js b/app/assets/javascripts/markdown_editor.js index 7369c356f..7f6c62710 100644 --- a/app/assets/javascripts/markdown_editor.js +++ b/app/assets/javascripts/markdown_editor.js @@ -28,8 +28,10 @@ editor.find("textarea").on("scroll", function() { editor.find(".markdown-preview").scrollTop($(this).scrollTop()); }); - editor.find(".fullscreen-toggle").on("click", function() { + editor.find(".fullscreen-toggle").on("click", function(e) { var span; + e.preventDefault(); + e.stopPropagation(); editor.toggleClass("fullscreen"); $(".fullscreen-container").toggleClass("medium-8", "medium-12"); span = $(this).find("span"); From f68553dc00372161b625f5c61d46d48c2950d356 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 18 Jun 2020 16:08:27 +0200 Subject: [PATCH 04/15] Extract method to fill in markdown editor in specs --- spec/support/common_actions/verifications.rb | 9 +++++++++ spec/system/admin/legislation/draft_versions_spec.rb | 9 +-------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/spec/support/common_actions/verifications.rb b/spec/support/common_actions/verifications.rb index d9c4b2c68..b6e65fae4 100644 --- a/spec/support/common_actions/verifications.rb +++ b/spec/support/common_actions/verifications.rb @@ -55,4 +55,13 @@ module Verifications within_frame(0) { find("body").set(with) } end end + + def fill_in_markdown_editor(label, with:) + click_link "Launch text editor" + fill_in label, with: with + + within(".fullscreen") do + click_link "Close text editor" + end + end end diff --git a/spec/system/admin/legislation/draft_versions_spec.rb b/spec/system/admin/legislation/draft_versions_spec.rb index e6c12238d..5ac47be22 100644 --- a/spec/system/admin/legislation/draft_versions_spec.rb +++ b/spec/system/admin/legislation/draft_versions_spec.rb @@ -83,14 +83,7 @@ describe "Admin legislation draft versions" do click_link "Version 1" fill_in "Version title", with: "Version 1b" - - click_link "Launch text editor" - - fill_in "Text", with: "# Version 1 body\r\n\r\nParagraph\r\n\r\n>Quote" - - within(".fullscreen") do - click_link "Close text editor" - end + fill_in_markdown_editor "Text", with: "# Version 1 body\r\n\r\nParagraph\r\n\r\n>Quote" click_button "Save changes" From cfc60b5de48aae534b57993ace1c59b68e22e2f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 18 Jun 2020 16:21:21 +0200 Subject: [PATCH 05/15] Warn for changes just in markdown editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the reason why this feature was implemented in the first place: it's easy to open the editor, make some changes, close it, and continue without realizing the changes have not been saved. In the rest of the forms, this functionality is quite lacking. For starters, some forms warn if there are unsaved changes, while some forms don't, which is highly inconsistent and disorients users. Furthermore, we were having problems with this feature after upgrading Turbolinks, particularly in forms using CKEditor. In these cases, a lot of hacking needs to be done in order to make this feature work properly, since CKEditor adds some formatting automatically, and if this is done after the form is serialized, we'll get some unexpected behavior. On the other hand, comparing the value of a textarea against its `defaultValue` property will work on every edge case, including using the browser's back button or reloading the page. Finally, users are used to the way web forms work, and aren't used to be asked for confirmation when they change their mind and decide to leave the page without saving the changes. Asking them for confirmation will be annoying in most cases. Besides that, if they accidentally leave the page, they can use the browser's back button and they'll recover the unsaved changes. It's true this won't happen it they accidentally close the browser's window, but our WatchFormChanges functionality didn't work in this case either. Using the "beforeunload" event adds more problems than it solves, since it doesn't support custom messages (or, to be more precise, modern browsers ignore custom messages), and it doesn't get along with turbolinks. Co-Authored-By: Senén Rodero Rodríguez --- app/assets/javascripts/application.js | 3 +- .../javascripts/legislation_draft_versions.js | 22 +++++++ app/assets/javascripts/watch_form_changes.js | 32 ----------- .../legislation/draft_versions/_form.html.erb | 2 +- .../legislation/draft_versions/edit.html.erb | 2 +- .../legislation/homepages/_form.html.erb | 2 +- .../legislation/processes/_form.html.erb | 2 +- .../legislation/proposals/_form.html.erb | 2 +- .../legislation/questions/_form.html.erb | 2 +- .../_form_content_block.html.erb | 2 +- .../site_customization/pages/_form.html.erb | 2 +- app/views/layouts/admin.html.erb | 2 +- config/locales/en/admin.yml | 1 + config/locales/en/general.yml | 2 - config/locales/es/admin.yml | 1 + config/locales/es/general.yml | 2 - .../admin/legislation/draft_versions_spec.rb | 57 +++++++++++++++++++ 17 files changed, 91 insertions(+), 47 deletions(-) create mode 100644 app/assets/javascripts/legislation_draft_versions.js delete mode 100644 app/assets/javascripts/watch_form_changes.js diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 4d1eb6aa2..9c1a7d602 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -87,7 +87,7 @@ //= require legislation //= require legislation_allegations //= require legislation_annotatable -//= require watch_form_changes +//= require legislation_draft_versions //= require followable //= require flaggable //= require documentable @@ -145,7 +145,6 @@ var initialize_modules = function() { if ($(".legislation-annotatable").length) { App.LegislationAnnotatable.initialize(); } - App.WatchFormChanges.initialize(); App.TreeNavigator.initialize(); App.Documentable.initialize(); App.Imageable.initialize(); diff --git a/app/assets/javascripts/legislation_draft_versions.js b/app/assets/javascripts/legislation_draft_versions.js new file mode 100644 index 000000000..742f3d492 --- /dev/null +++ b/app/assets/javascripts/legislation_draft_versions.js @@ -0,0 +1,22 @@ +(function() { + "use strict"; + App.LegislationDraftVersions = { + msg: function() { + return $("[data-markdown-changes-message]").data("markdown-changes-message"); + }, + hasChanged: function() { + return $(".markdown-editor textarea").is(function() { + return this.value !== this.defaultValue; + }); + }, + checkChanges: function() { + if (App.LegislationDraftVersions.hasChanged()) { + return confirm(App.LegislationDraftVersions.msg()); + } else { + return true; + } + }, + }; + + $(document).on("turbolinks:before-visit", App.LegislationDraftVersions.checkChanges); +}).call(this); diff --git a/app/assets/javascripts/watch_form_changes.js b/app/assets/javascripts/watch_form_changes.js deleted file mode 100644 index b666ce7ac..000000000 --- a/app/assets/javascripts/watch_form_changes.js +++ /dev/null @@ -1,32 +0,0 @@ -(function() { - "use strict"; - App.WatchFormChanges = { - forms: function() { - return $("form[data-watch-changes]"); - }, - msg: function() { - return $("[data-watch-form-message]").data("watch-form-message"); - }, - hasChanged: function() { - return App.WatchFormChanges.forms().is(function() { - return $(this).serialize() !== $(this).data("watchChanges"); - }); - }, - checkChanges: function() { - if (App.WatchFormChanges.hasChanged()) { - return confirm(App.WatchFormChanges.msg()); - } else { - return true; - } - }, - initialize: function() { - if (App.WatchFormChanges.forms().length === 0 || App.WatchFormChanges.msg() === undefined) { - return; - } - $(document).off("turbolinks:before-visit").on("turbolinks:before-visit", App.WatchFormChanges.checkChanges); - App.WatchFormChanges.forms().each(function() { - $(this).data("watchChanges", $(this).serialize()); - }); - } - }; -}).call(this); diff --git a/app/views/admin/legislation/draft_versions/_form.html.erb b/app/views/admin/legislation/draft_versions/_form.html.erb index 3d500214c..678f8862b 100644 --- a/app/views/admin/legislation/draft_versions/_form.html.erb +++ b/app/views/admin/legislation/draft_versions/_form.html.erb @@ -1,6 +1,6 @@ <%= render "shared/globalize_locales", resource: @draft_version %> -<%= translatable_form_for [:admin, @process, @draft_version], url: url, html: { data: { watch_changes: true }} do |f| %> +<%= translatable_form_for [:admin, @process, @draft_version], url: url do |f| %> <%= render "shared/errors", resource: @draft_version %> diff --git a/app/views/admin/legislation/draft_versions/edit.html.erb b/app/views/admin/legislation/draft_versions/edit.html.erb index f9b9593a5..537a671e0 100644 --- a/app/views/admin/legislation/draft_versions/edit.html.erb +++ b/app/views/admin/legislation/draft_versions/edit.html.erb @@ -12,7 +12,7 @@ <%= render "admin/legislation/processes/subnav", process: @process, active: "draft_versions" %>
- diff --git a/app/views/admin/legislation/homepages/_form.html.erb b/app/views/admin/legislation/homepages/_form.html.erb index a0758efed..4a752569c 100644 --- a/app/views/admin/legislation/homepages/_form.html.erb +++ b/app/views/admin/legislation/homepages/_form.html.erb @@ -3,7 +3,7 @@ display_style: lambda { |locale| enable_translation_style(@process, locale) }, manage_languages: false %> -<%= translatable_form_for [:admin, @process], url: url, html: { data: { watch_changes: true } } do |f| %> +<%= translatable_form_for [:admin, @process], url: url do |f| %> <%= render "shared/errors", resource: @process %>
diff --git a/app/views/admin/legislation/processes/_form.html.erb b/app/views/admin/legislation/processes/_form.html.erb index bbcfc2f8a..9c5945334 100644 --- a/app/views/admin/legislation/processes/_form.html.erb +++ b/app/views/admin/legislation/processes/_form.html.erb @@ -1,6 +1,6 @@ <%= render "shared/globalize_locales", resource: @process %> -<%= translatable_form_for [:admin, @process], html: { data: { watch_changes: true } } do |f| %> +<%= translatable_form_for [:admin, @process] do |f| %> <%= render "shared/errors", resource: @process %> diff --git a/app/views/admin/legislation/proposals/_form.html.erb b/app/views/admin/legislation/proposals/_form.html.erb index 5d1a14370..494d3fe14 100644 --- a/app/views/admin/legislation/proposals/_form.html.erb +++ b/app/views/admin/legislation/proposals/_form.html.erb @@ -1,4 +1,4 @@ -<%= form_for [:admin, @process], html: { data: { watch_changes: true } } do |f| %> +<%= form_for [:admin, @process] do |f| %> <%= render "shared/errors", resource: @process %> diff --git a/app/views/admin/legislation/questions/_form.html.erb b/app/views/admin/legislation/questions/_form.html.erb index 1b43c79ac..45c9268d6 100644 --- a/app/views/admin/legislation/questions/_form.html.erb +++ b/app/views/admin/legislation/questions/_form.html.erb @@ -1,6 +1,6 @@ <%= render "shared/globalize_locales", resource: @question %> -<%= translatable_form_for [:admin, @process, @question], url: url, html: { data: { watch_changes: true } } do |f| %> +<%= translatable_form_for [:admin, @process, @question], url: url do |f| %> <%= render "shared/errors", resource: @question %> diff --git a/app/views/admin/site_customization/content_blocks/_form_content_block.html.erb b/app/views/admin/site_customization/content_blocks/_form_content_block.html.erb index a2bb04c3c..01f61438e 100644 --- a/app/views/admin/site_customization/content_blocks/_form_content_block.html.erb +++ b/app/views/admin/site_customization/content_blocks/_form_content_block.html.erb @@ -1,4 +1,4 @@ -<%= form_for [:admin, @content_block], html: { class: "edit_page", data: { watch_changes: true } } do |f| %> +<%= form_for [:admin, @content_block], html: { class: "edit_page" } do |f| %> <%= render "shared/errors", resource: @content_block %> diff --git a/app/views/admin/site_customization/pages/_form.html.erb b/app/views/admin/site_customization/pages/_form.html.erb index 76c1d175e..a30395fc6 100644 --- a/app/views/admin/site_customization/pages/_form.html.erb +++ b/app/views/admin/site_customization/pages/_form.html.erb @@ -1,6 +1,6 @@ <%= render "shared/globalize_locales", resource: @page %> -<%= translatable_form_for [:admin, @page], html: { class: "edit_page", data: { watch_changes: true } } do |f| %> +<%= translatable_form_for [:admin, @page], html: { class: "edit_page" } do |f| %> <%= render "shared/errors", resource: @page %>
diff --git a/app/views/layouts/admin.html.erb b/app/views/layouts/admin.html.erb index e5d132669..b73dc8fae 100644 --- a/app/views/layouts/admin.html.erb +++ b/app/views/layouts/admin.html.erb @@ -6,7 +6,7 @@ <%= content_for :head %> - "> +
diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 61f1eca24..6986aecb0 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -553,6 +553,7 @@ en: back: Back submit_button: Save changes warning: You've edited the text, don't forget to click on Save to permanently save the changes. + markdown_changes_message: "You've edited the text without saving it. Do you confirm to leave the page?" form: title: 'Editing %{draft_version_title} from the process %{process_title}' launch_text_editor: Launch text editor diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 24fb4fe6f..d24df2a99 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -252,8 +252,6 @@ en: other: You have %{count} new notifications notifications: Notifications no_notifications: "You don't have new notifications" - admin: - watch_form_message: "You have unsaved changes. Do you confirm to leave the page?" notifications: index: empty_notifications: You don't have new notifications. diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 0978bd7f7..e6930a50a 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -552,6 +552,7 @@ es: back: Volver submit_button: Guardar cambios warning: Ojo, has editado el texto. Para conservar de forma permanente los cambios, no te olvides de hacer click en Guardar. + markdown_changes_message: "Ojo, has editado el texto; perderás los cambios si abandonas la página. ¿Seguro que quieres continuar?" form: title: 'Editando %{draft_version_title} del proceso %{process_title}' launch_text_editor: Lanzar editor de texto diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index 92de921d8..e6444bf60 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -252,8 +252,6 @@ es: other: Tienes %{count} notificaciones nuevas notifications: Notificaciones no_notifications: "No tienes notificaciones nuevas" - admin: - watch_form_message: "Has realizado cambios que no han sido guardados. ¿Seguro que quieres abandonar la página?" notifications: index: empty_notifications: No tienes notificaciones nuevas. diff --git a/spec/system/admin/legislation/draft_versions_spec.rb b/spec/system/admin/legislation/draft_versions_spec.rb index 5ac47be22..a869d7a0f 100644 --- a/spec/system/admin/legislation/draft_versions_spec.rb +++ b/spec/system/admin/legislation/draft_versions_spec.rb @@ -90,4 +90,61 @@ describe "Admin legislation draft versions" do expect(page).to have_content "Version 1b" end end + + context "Changing content with the markdown editor", :js do + let(:prompt) { "You've edited the text without saving it. Do you confirm to leave the page?" } + let(:version) { create(:legislation_draft_version, body: "Version 1") } + let(:path) do + edit_admin_legislation_process_draft_version_path(version.process, version) + end + + scenario "asks for confimation when the content is modified" do + visit path + fill_in_markdown_editor "Text", with: "Version 1b" + + dismiss_confirm(prompt) do + click_link "Proposals", match: :first + end + + expect(page).to have_current_path(path) + end + + scenario "asks for confimation after the page is restored from browser history" do + visit path + fill_in_markdown_editor "Text", with: "Version 1b" + + accept_confirm(prompt) do + click_link "Proposals", match: :first + end + + expect(page).to have_css("h2", text: "Proposals") + + go_back + + expect(page).to have_content version.process.title + + accept_confirm(prompt) do + click_link "Proposals", match: :first + end + + expect(page).to have_css("h2", text: "Proposals") + end + + scenario "does not ask for confirmation when restoring the original content" do + visit path + fill_in_markdown_editor "Text", with: "Version 1b" + + accept_confirm(prompt) do + click_link "Proposals", match: :first + end + + expect(page).to have_css("h2", text: "Proposals") + + go_back + fill_in_markdown_editor "Text", with: "Version 1" + click_link "Proposals", match: :first + + expect(page).to have_css("h2", text: "Proposals") + end + end end From 0270c4c96204bf32bc840948ce7f15a0845cd642 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 18 Jun 2020 20:08:49 +0200 Subject: [PATCH 06/15] Hide unsaved changes warning after undoing them This way showing the warning is consistent with warning users when they're leaving the page. --- app/assets/javascripts/markdown_editor.js | 12 +++++++++++- .../admin/legislation/draft_versions/_form.html.erb | 9 ++++++++- .../admin/legislation/draft_versions/edit.html.erb | 6 ------ spec/system/admin/legislation/draft_versions_spec.rb | 6 ++++++ 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/markdown_editor.js b/app/assets/javascripts/markdown_editor.js index 7f6c62710..d8e34719f 100644 --- a/app/assets/javascripts/markdown_editor.js +++ b/app/assets/javascripts/markdown_editor.js @@ -22,8 +22,18 @@ }); editor = $(this); editor.on("input", function() { + var textarea, warning; + + textarea = editor.find("textarea")[0]; + warning = $(this).closest(".translatable-fields").find(".warning"); + App.MarkdownEditor.refresh_preview($(this), md); - $(".legislation-draft-versions-edit .warning").show(); + + if (textarea.value === textarea.defaultValue) { + warning.hide(); + } else { + warning.show(); + } }); editor.find("textarea").on("scroll", function() { editor.find(".markdown-preview").scrollTop($(this).scrollTop()); diff --git a/app/views/admin/legislation/draft_versions/_form.html.erb b/app/views/admin/legislation/draft_versions/_form.html.erb index 678f8862b..9298990cd 100644 --- a/app/views/admin/legislation/draft_versions/_form.html.erb +++ b/app/views/admin/legislation/draft_versions/_form.html.erb @@ -1,11 +1,18 @@ <%= render "shared/globalize_locales", resource: @draft_version %> -<%= translatable_form_for [:admin, @process, @draft_version], url: url do |f| %> +<%= translatable_form_for [:admin, @process, @draft_version], url: url, + html: { data: { markdown_changes_message: I18n.t("admin.legislation.draft_versions.edit.markdown_changes_message") }} do |f| %> <%= render "shared/errors", resource: @draft_version %>
<%= f.translatable_fields do |translations_form| %> +
+ +
+
<%= translations_form.text_field :title, placeholder: t("admin.legislation.draft_versions.form.title_placeholder") %> diff --git a/app/views/admin/legislation/draft_versions/edit.html.erb b/app/views/admin/legislation/draft_versions/edit.html.erb index 537a671e0..a49b9dbd2 100644 --- a/app/views/admin/legislation/draft_versions/edit.html.erb +++ b/app/views/admin/legislation/draft_versions/edit.html.erb @@ -11,12 +11,6 @@ <%= render "admin/legislation/processes/subnav", process: @process, active: "draft_versions" %> -
-
" style="display: none;"> - <%= t("admin.legislation.draft_versions.edit.warning") %> -
-
-

<%= @draft_version.title %>

diff --git a/spec/system/admin/legislation/draft_versions_spec.rb b/spec/system/admin/legislation/draft_versions_spec.rb index a869d7a0f..05e41194c 100644 --- a/spec/system/admin/legislation/draft_versions_spec.rb +++ b/spec/system/admin/legislation/draft_versions_spec.rb @@ -102,6 +102,8 @@ describe "Admin legislation draft versions" do visit path fill_in_markdown_editor "Text", with: "Version 1b" + expect(page).to have_content "You've edited the text" + dismiss_confirm(prompt) do click_link "Proposals", match: :first end @@ -141,7 +143,11 @@ describe "Admin legislation draft versions" do expect(page).to have_css("h2", text: "Proposals") go_back + fill_in_markdown_editor "Text", with: "Version 1" + + expect(page).not_to have_content "You've edited the text" + click_link "Proposals", match: :first expect(page).to have_css("h2", text: "Proposals") From 6bb9ebf12ff1e414462b3ca54f23f71cd0ed4ac9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 3 Aug 2020 14:08:26 +0200 Subject: [PATCH 07/15] Move before-cache event calls to application.js Turbolinks 5 handles page caching differently; it saves the current HTML and, when the page is restored using the browser's back button, it copies it and triggers the `turbolinks:load` event, which, in our case, triggers our `initialize_modules` function. This isn't a problem regarding event handlers, since they're removed when caching the page (except fot the ones attached to the `document`). However, it is a problem for functions that, once the document is ready, scan the DOM and add certain elements. In this case, an element might be added a second time when the page is restored. So we need to either check an item has already been added before adding it or remove it before caching the page. Since this is going to be a common pattern, we're adding a function to handle these non-idempotent parts of the application, so it mirrors our `initialize_modules` function. We're also moving the `destroy` function definition after the `initialize` function definition, which makes more sense since we initialize things before destroying them. --- app/assets/javascripts/application.js | 7 +++++++ app/assets/javascripts/html_editor.js | 12 +++++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 9c1a7d602..776f9931e 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -166,4 +166,11 @@ var initialize_modules = function() { App.BudgetEditAssociations.initialize(); }; +var destroy_non_idempotent_modules = function() { + "use strict"; + + App.HTMLEditor.destroy(); +}; + $(document).on("turbolinks:load", initialize_modules); +$(document).on("turbolinks:before-cache", destroy_non_idempotent_modules); diff --git a/app/assets/javascripts/html_editor.js b/app/assets/javascripts/html_editor.js index affdc7034..e1602509d 100644 --- a/app/assets/javascripts/html_editor.js +++ b/app/assets/javascripts/html_editor.js @@ -1,11 +1,6 @@ (function() { "use strict"; App.HTMLEditor = { - destroy: function() { - for (var name in CKEDITOR.instances) { - CKEDITOR.instances[name].destroy(); - } - }, initialize: function() { $("textarea.html-area").each(function() { if ($(this).hasClass("admin")) { @@ -14,8 +9,11 @@ CKEDITOR.replace(this.name, { language: $("html").attr("lang") }); } }); + }, + destroy: function() { + for (var name in CKEDITOR.instances) { + CKEDITOR.instances[name].destroy(); + } } }; - - $(document).on("turbolinks:before-cache", App.HTMLEditor.destroy); }).call(this); From da658f3d8c3ed6f324f253c4c4fe6bdfd99a300a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Thu, 2 Jul 2020 12:24:32 +0200 Subject: [PATCH 08/15] Hack datepicker to make it work with Turbolinks 5.x Patch extracted from here the comments on turbolinks issue 253 and converted to vanilla javascript. The hide action over datepickers ensures us that opened datepickers will be closed before leving the page. Previously if you open any datepicker and then move to previous page you will keep seeing the datepicker in the restored page. --- app/assets/javascripts/application.js | 2 ++ app/assets/javascripts/datepicker.js | 31 +++++++++++++++++++++++++++ spec/system/admin/banners_spec.rb | 28 ++++++++++++++++++++++++ 3 files changed, 61 insertions(+) create mode 100644 app/assets/javascripts/datepicker.js diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 776f9931e..512d89bfd 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -111,6 +111,7 @@ //= require cookies //= require columns_selector //= require budget_edit_associations +//= require datepicker var initialize_modules = function() { "use strict"; @@ -169,6 +170,7 @@ var initialize_modules = function() { var destroy_non_idempotent_modules = function() { "use strict"; + App.Datepicker.destroy(); App.HTMLEditor.destroy(); }; diff --git a/app/assets/javascripts/datepicker.js b/app/assets/javascripts/datepicker.js new file mode 100644 index 000000000..b44fa8e58 --- /dev/null +++ b/app/assets/javascripts/datepicker.js @@ -0,0 +1,31 @@ +// Based on code by Javan Makhmali +// https://github.com/turbolinks/turbolinks/issues/253#issuecomment-289101048 +// The jQuery UI date picker widget appends a shared element to the +// body which it expects will never leave the page, but Turbolinks +// removes that shared element when it rerenders. We satisfy that +// expectation by removing the shared element from the page before +// Turbolinks caches the page, and appending it again before +// Turbolinks swaps the new body in during rendering. +// +// Additionally, returning to the cached version of a page that +// previously had date picker elements would result in those date +// pickers not being initialized again. We fix this issue by finding +// all initialized date picker inputs on the page and calling the +// date picker's destroy method before Turbolinks caches the page. +(function() { + "use strict"; + App.Datepicker = { + destroy: function() { + $.datepicker.dpDiv.remove(); + + document.querySelectorAll("input.hasDatepicker").forEach(function(input) { + $(input).datepicker("hide"); + $(input).datepicker("destroy"); + }); + } + }; + + document.addEventListener("turbolinks:before-render", function(event) { + $.datepicker.dpDiv.appendTo(event.data.newBody); + }); +}).call(this); diff --git a/spec/system/admin/banners_spec.rb b/spec/system/admin/banners_spec.rb index 3b30f792b..9cd5e643c 100644 --- a/spec/system/admin/banners_spec.rb +++ b/spec/system/admin/banners_spec.rb @@ -190,6 +190,34 @@ describe "Admin banners magement" do expect(page).to have_field "Post started at", with: "22/02/2002" end + scenario "when date picker is opened and click on browser history back datepicker is closed", :js do + banner = create(:banner) + visit admin_banners_path(banner) + click_link "Edit banner" + find_field("Post started at").click + + expect(page).to have_css "#ui-datepicker-div" + + go_back + + expect(page).to have_content("Banners") + expect(page).not_to have_css "#ui-datepicker-div" + end + + scenario "date picker works after using the browser back button", :js do + banner = create(:banner) + + visit edit_admin_banner_path(banner) + click_link "Manage banners" + + expect(page).to have_link "Edit banner" + + go_back + find_field("Post started at").click + + expect(page).to have_css "#ui-datepicker-div" + end + scenario "Delete a banner" do create(:banner, title: "Ugly banner", description: "Bad text", From fc0625df8b6465e2c1137b0932870f4275dea7c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 10 Jul 2020 16:33:03 +0200 Subject: [PATCH 09/15] Destroy Annotator app before storing page into brwoser cache If we do not destroy annotator app before storing the page at browser cache we will unnecesarily initialize annotations twice (or more) duplicating Annotator HTML markup and causing unexpected errors. Without this commit you will find an error when restoring a page with annotator, you can click on any annotation and you will see the annotation comments are being loaded twice. IMO this is an idempotency issue within Annotator JS library. --- app/assets/javascripts/application.js | 1 + .../javascripts/legislation_annotatable.js | 5 +++++ spec/system/legislation/draft_versions_spec.rb | 17 +++++++++++++++++ 3 files changed, 23 insertions(+) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 512d89bfd..d9298762b 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -172,6 +172,7 @@ var destroy_non_idempotent_modules = function() { App.Datepicker.destroy(); App.HTMLEditor.destroy(); + App.LegislationAnnotatable.destroy(); }; $(document).on("turbolinks:load", initialize_modules); diff --git a/app/assets/javascripts/legislation_annotatable.js b/app/assets/javascripts/legislation_annotatable.js index 9b1125722..9ab0f6d6e 100644 --- a/app/assets/javascripts/legislation_annotatable.js +++ b/app/assets/javascripts/legislation_annotatable.js @@ -226,6 +226,11 @@ }); }); }); + }, + destroy: function() { + if ($(".legislation-annotatable").length > 0) { + App.LegislationAnnotatable.app.destroy(); + } } }; }).call(this); diff --git a/spec/system/legislation/draft_versions_spec.rb b/spec/system/legislation/draft_versions_spec.rb index c3b3bb687..f2914b7d1 100644 --- a/spec/system/legislation/draft_versions_spec.rb +++ b/spec/system/legislation/draft_versions_spec.rb @@ -237,6 +237,23 @@ describe "Legislation Draft Versions" do expect(page).to have_content "Comment can't be blank" end + + scenario "When page is restored from browser cache do not duplicate annotation handlers" do + create(:legislation_annotation, draft_version: draft_version, text: "my annotation") + + visit legislation_process_draft_version_path(draft_version.process, draft_version) + + expect(page).to have_css(".annotator-hl", count: 1) + + click_link "Help" + + expect(page).to have_content "CONSUL is a platform for citizen participation" + + go_back + + expect(page).to have_content "A collaborative legislation process" + expect(page).to have_css(".annotator-hl", count: 1) + end end context "Merged annotations", :js do From 6b17452bd51db903dc98a982a36a90fc2e187de5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 10 Jul 2020 17:28:41 +0200 Subject: [PATCH 10/15] Fix "Publish comment" button when restoring a page from browser cache We need to use page body event delegation so it will work with any element even with the ones added through ajax, in this case the annotation comments box form. By doing this way we do not need this code on the server response anymore. Furthermore JS events defined at ajax responses are not part of application javascript and are lost when restoring a page from browser cache, you can try to apply the same event delegation technique to the `erb` file and it wont work just because events added dinamically are not treated the same than `application.js` code. To reproduce the error: 1. Load an annotatable draft version 2. Move to any other page 3. Go back Now "Publish comment" button wont work. --- .../javascripts/legislation_annotatable.js | 11 ++++++++++ .../annotations/_comments_box.html.erb | 2 +- .../annotations/_comments_box_form.js.erb | 10 --------- .../system/legislation/draft_versions_spec.rb | 22 +++++++++++++++++++ 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/legislation_annotatable.js b/app/assets/javascripts/legislation_annotatable.js index 9ab0f6d6e..1b0c91ca7 100644 --- a/app/assets/javascripts/legislation_annotatable.js +++ b/app/assets/javascripts/legislation_annotatable.js @@ -186,6 +186,15 @@ } }; }, + initCommentFormToggler: function() { + $("body").on("click", ".comment-box a.publish-comment", function(e) { + e.preventDefault(); + var annotation_id = $(this).closest(".comment-box").data("id"); + $("a.publish-comment").hide(); + $("#js-comment-form-annotation-" + annotation_id).toggle(); + $("#js-comment-form-annotation-" + annotation_id + " textarea").trigger("focus"); + }); + }, initialize: function() { var current_user_id; $("body").on("renderLegislationAnnotation", App.LegislationAnnotatable.renderAnnotationComments); @@ -226,6 +235,8 @@ }); }); }); + + App.LegislationAnnotatable.initCommentFormToggler(); }, destroy: function() { if ($(".legislation-annotatable").length > 0) { diff --git a/app/views/legislation/annotations/_comments_box.html.erb b/app/views/legislation/annotations/_comments_box.html.erb index c1d1f24fd..6d5661622 100644 --- a/app/views/legislation/annotations/_comments_box.html.erb +++ b/app/views/legislation/annotations/_comments_box.html.erb @@ -1,4 +1,4 @@ -
+
<%= render "comment_header", annotation: annotation %>
diff --git a/app/views/legislation/annotations/_comments_box_form.js.erb b/app/views/legislation/annotations/_comments_box_form.js.erb index b460b32b0..fbfce4c33 100644 --- a/app/views/legislation/annotations/_comments_box_form.js.erb +++ b/app/views/legislation/annotations/_comments_box_form.js.erb @@ -1,13 +1,3 @@ -$("#comments-box-<%= annotation.id %> a.publish-comment").on({ - click: function(e) { - e.preventDefault(); - $("a.publish-comment").hide(); - $("#js-comment-form-annotation-<%= annotation.id %>").toggle(); - $("#js-comment-form-annotation-<%= annotation.id %> textarea").focus(); - return; - } -}); - <% if comment.errors.any? %> $("#comments-box-<%= @annotation.id %> a.publish-comment").hide(); $("#js-comment-form-annotation-<%= annotation.id %>").toggle(); diff --git a/spec/system/legislation/draft_versions_spec.rb b/spec/system/legislation/draft_versions_spec.rb index f2914b7d1..a2b4ff259 100644 --- a/spec/system/legislation/draft_versions_spec.rb +++ b/spec/system/legislation/draft_versions_spec.rb @@ -254,6 +254,28 @@ describe "Legislation Draft Versions" do expect(page).to have_content "A collaborative legislation process" expect(page).to have_css(".annotator-hl", count: 1) end + + scenario "When page is restored from browser cache publish comment button keeps working" do + create(:legislation_annotation, draft_version: draft_version, text: "my annotation") + + visit legislation_process_draft_version_path(draft_version.process, draft_version) + + find(:css, ".annotator-hl").click + + click_link "Help" + + expect(page).to have_content "CONSUL is a platform for citizen participation" + + go_back + + expect(page).to have_content "A collaborative legislation process" + + click_link "Publish Comment" + fill_in "comment[body]", with: "My interesting comment" + click_button "Publish comment" + + expect(page).to have_content "My interesting comment" + end end context "Merged annotations", :js do From 137e0f5a64e11f2bc36dfdd641789e536a2fc666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Mon, 27 Jul 2020 12:53:33 +0200 Subject: [PATCH 11/15] Remove description for screen readers It was being duplicated when restoring a page by using browser history. With this solution we will avoid to have screen readers descriptions more than once inside any sociual share button. --- app/assets/javascripts/application.js | 1 + app/assets/javascripts/social_share.js | 3 +++ spec/system/proposals_spec.rb | 12 ++++++++++++ 3 files changed, 16 insertions(+) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index d9298762b..184fb3aa2 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -173,6 +173,7 @@ var destroy_non_idempotent_modules = function() { App.Datepicker.destroy(); App.HTMLEditor.destroy(); App.LegislationAnnotatable.destroy(); + App.SocialShare.destroy(); }; $(document).on("turbolinks:load", initialize_modules); diff --git a/app/assets/javascripts/social_share.js b/app/assets/javascripts/social_share.js index 34ae229ac..eb75fe479 100644 --- a/app/assets/javascripts/social_share.js +++ b/app/assets/javascripts/social_share.js @@ -5,6 +5,9 @@ $(".social-share-button a").each(function() { $(this).append("" + ($(this).data("site")) + ""); }); + }, + destroy: function() { + $(".social-share-button a .show-for-sr").remove(); } }; }).call(this); diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index d0978fa18..cd84e2c5b 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -198,6 +198,18 @@ describe "Proposals" do expect(page).not_to have_link("No comments", href: "#comments") end end + + scenario "After using the browser's back button, social buttons will have one screen reader", :js do + proposal = create(:proposal) + visit proposal_path(proposal) + click_link "Help" + + expect(page).to have_content "CONSUL is a platform for citizen participation" + + go_back + + expect(page).to have_css "span.show-for-sr", text: "twitter", count: 1 + end end describe "Show sticky support button on mobile screens", :js do From 87339451de18fdf26d1ddfed2b66bbfedebe548b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Tue, 28 Jul 2020 14:11:08 +0200 Subject: [PATCH 12/15] Reset columns_selector before caching the page This will allow to initialize this module again without duplicating columns checkboxes and without breaking the page. --- app/assets/javascripts/application.js | 1 + app/assets/javascripts/columns_selector.js | 3 +++ spec/system/admin/budget_investments_spec.rb | 22 ++++++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 184fb3aa2..65ec8687f 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -170,6 +170,7 @@ var initialize_modules = function() { var destroy_non_idempotent_modules = function() { "use strict"; + App.ColumnsSelector.destroy(); App.Datepicker.destroy(); App.HTMLEditor.destroy(); App.LegislationAnnotatable.destroy(); diff --git a/app/assets/javascripts/columns_selector.js b/app/assets/javascripts/columns_selector.js index 77dd5f4c1..2f680b7c6 100644 --- a/app/assets/javascripts/columns_selector.js +++ b/app/assets/javascripts/columns_selector.js @@ -85,6 +85,9 @@ $(".column-selectable").on("inserted", function() { App.ColumnsSelector.initColumns(); }); + }, + destroy: function() { + $("#js-columns-selector-wrapper").children(":not(#column_selector_item_template)").remove(); } }; }).call(this); diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index d2b0d67a9..8b2f1f72b 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1877,5 +1877,27 @@ describe "Admin budget investments" do expect(page).not_to have_content "Don't display me, please!" end end + + scenario "When restoring the page from browser history renders columns selectors only once", :js do + visit admin_budget_budget_investments_path(budget) + + click_link "Proposals" + + expect(page).to have_content("There are no proposals.") + + go_back + + within("#js-columns-selector") do + find("strong", text: "Columns").click + end + + within("#js-columns-selector-wrapper") do + selectable_columns.each do |column| + check_text = I18n.t("admin.budget_investments.index.list.#{column}") + + expect(page).to have_content(check_text, count: 1) + end + end + end end end From 289426c1c3d3ee6433b27c0e0d52e0262d45fb68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Wed, 29 Jul 2020 14:53:57 +0200 Subject: [PATCH 13/15] Destroy maps before leaving the current page If we do not do this a map could be initialized twice times or more when restoring a page with a map causing weird UI effects and loading some map layers also twice times or more. Need to add a maps array to be able to store all initialized (visible) maps so we can destroy them when needed. Notice that we are destroying maps also when admin settings tabs changes (only visible ones), this is again to avoid to re-initialize map more than once when users navigate through settings tabs, another option to the settings issue could be to detect if the map was already initialized to skip uneeded initialization. --- app/assets/javascripts/application.js | 1 + app/assets/javascripts/map.js | 9 +++++++++ app/assets/javascripts/settings.js | 6 +----- spec/shared/system/mappable.rb | 23 +++++++++++++++++++++++ 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 65ec8687f..5e7842945 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -174,6 +174,7 @@ var destroy_non_idempotent_modules = function() { App.Datepicker.destroy(); App.HTMLEditor.destroy(); App.LegislationAnnotatable.destroy(); + App.Map.destroy(); App.SocialShare.destroy(); }; diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index d160d3a92..9d864d585 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -1,6 +1,7 @@ (function() { "use strict"; App.Map = { + maps: [], initialize: function() { $("*[data-map]:visible").each(function() { App.Map.initializeMap(this); @@ -11,6 +12,13 @@ } }); }, + destroy: function() { + App.Map.maps.forEach(function(map) { + map.off(); + map.remove(); + }); + App.Map.maps = []; + }, initializeMap: function(element) { var addMarkerInvestments, clearFormfields, createMarker, editable, getPopupContent, latitudeInputSelector, longitudeInputSelector, map, mapAttribution, mapCenterLatLng, mapCenterLatitude, mapCenterLongitude, mapTilesProvider, marker, markerIcon, markerLatitude, markerLongitude, moveOrPlaceMarker, openMarkerPopup, removeMarker, removeMarkerSelector, updateFormfields, zoom, zoomInputSelector; App.Map.cleanInvestmentCoordinates(element); @@ -88,6 +96,7 @@ }; mapCenterLatLng = new L.LatLng(mapCenterLatitude, mapCenterLongitude); map = L.map(element.id).setView(mapCenterLatLng, zoom); + App.Map.maps.push(map); L.tileLayer(mapTilesProvider, { attribution: mapAttribution }).addTo(map); diff --git a/app/assets/javascripts/settings.js b/app/assets/javascripts/settings.js index cea8fcda7..57be249ae 100644 --- a/app/assets/javascripts/settings.js +++ b/app/assets/javascripts/settings.js @@ -3,12 +3,8 @@ App.Settings = { initialize: function() { $("#settings-tabs").on("change.zf.tabs", function() { - var map_container; if ($("#tab-map-configuration:visible").length) { - map_container = L.DomUtil.get("admin-map"); - if (map_container !== null) { - map_container._leaflet_id = null; - } + App.Map.destroy(); App.Map.initialize(); } }); diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index 063b02851..455e5876e 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -79,6 +79,29 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, expect(page).to have_content "Map location can't be blank" end + describe "When restoring the page from browser history" do + scenario "map should not be duplicated", :js do + do_login_for user + visit send(mappable_new_path, arguments) + + if management + click_link "Select user" + + expect(page).to have_content "User management" + else + click_link "Help" + + expect(page).to have_content "CONSUL is a platform for citizen participation" + end + + go_back + + within ".map_location" do + expect(page).to have_css(".leaflet-map-pane", count: 1) + end + end + end + scenario "Skip map", :js do do_login_for user visit send(mappable_new_path, arguments) From 6aa94a787ce221a7c808e5085bc48b8ce82c3069 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Tue, 28 Jul 2020 09:58:30 +0200 Subject: [PATCH 14/15] Use map location form latitude, longitude and zoom when valid When using an editable map is better to load marker latitude, longitude and map zoom from form fields so we can show the marker at latest position defined by user when the page was restored from browser history. To reproduce this behavior: 0. Undo this commit 1. Go to new proposal page 2. Place the proposal map marker 3. Go away to any other page 4. Restore new proposal page from browser history. At this point you should not see the recently placed marker. The same thing happens when editing a proposal. --- app/assets/javascripts/map.js | 29 +++++++++++++++++++---- spec/shared/system/mappable.rb | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index 9d864d585..0dc8d15d3 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -20,18 +20,36 @@ App.Map.maps = []; }, initializeMap: function(element) { - var addMarkerInvestments, clearFormfields, createMarker, editable, getPopupContent, latitudeInputSelector, longitudeInputSelector, map, mapAttribution, mapCenterLatLng, mapCenterLatitude, mapCenterLongitude, mapTilesProvider, marker, markerIcon, markerLatitude, markerLongitude, moveOrPlaceMarker, openMarkerPopup, removeMarker, removeMarkerSelector, updateFormfields, zoom, zoomInputSelector; + var addMarkerInvestments, clearFormfields, createMarker, editable, formCoordinates, getPopupContent, + latitudeInputSelector, longitudeInputSelector, map, mapAttribution, mapCenterLatLng, + mapCenterLatitude, mapCenterLongitude, mapTilesProvider, marker, markerIcon, markerLatitude, + markerLongitude, moveOrPlaceMarker, openMarkerPopup, removeMarker, removeMarkerSelector, + updateFormfields, zoom, zoomInputSelector; App.Map.cleanInvestmentCoordinates(element); mapCenterLatitude = $(element).data("map-center-latitude"); mapCenterLongitude = $(element).data("map-center-longitude"); - markerLatitude = $(element).data("marker-latitude"); - markerLongitude = $(element).data("marker-longitude"); - zoom = $(element).data("map-zoom"); mapTilesProvider = $(element).data("map-tiles-provider"); mapAttribution = $(element).data("map-tiles-provider-attribution"); latitudeInputSelector = $(element).data("latitude-input-selector"); longitudeInputSelector = $(element).data("longitude-input-selector"); zoomInputSelector = $(element).data("zoom-input-selector"); + formCoordinates = { + lat: $(latitudeInputSelector).val(), + long: $(longitudeInputSelector).val(), + zoom: $(zoomInputSelector).val() + }; + if (App.Map.validCoordinates(formCoordinates)) { + markerLatitude = formCoordinates.lat; + markerLongitude = formCoordinates.long; + } else { + markerLatitude = $(element).data("marker-latitude"); + markerLongitude = $(element).data("marker-longitude"); + } + if (App.Map.validZoom(formCoordinates.zoom)) { + zoom = formCoordinates.zoom; + } else { + zoom = $(element).data("map-zoom"); + } removeMarkerSelector = $(element).data("marker-remove-selector"); addMarkerInvestments = $(element).data("marker-investments-coordinates"); editable = $(element).data("marker-editable"); @@ -134,6 +152,9 @@ $(element).attr("data-marker-investments-coordinates", clean_markers); } }, + validZoom: function(zoom) { + return App.Map.isNumeric(zoom); + }, validCoordinates: function(coordinates) { return App.Map.isNumeric(coordinates.lat) && App.Map.isNumeric(coordinates.long); }, diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index 455e5876e..8d1df1aab 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -100,6 +100,40 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, expect(page).to have_css(".leaflet-map-pane", count: 1) end end + + scenario "keeps marker and zoom defined by the user", :js do + do_login_for user + visit send(mappable_new_path, arguments) + + within ".map_location" do + expect(page).not_to have_css(".map-icon") + end + expect(page.execute_script("return App.Map.maps[0].getZoom();")).to eq(10) + + map_zoom_in + find("#new_map_location").click + + within ".map_location" do + expect(page).to have_css(".map-icon") + end + + if management + click_link "Select user" + + expect(page).to have_content "User management" + else + click_link "Help" + + expect(page).to have_content "CONSUL is a platform for citizen participation" + end + + go_back + + within ".map_location" do + expect(page).to have_css(".map-icon") + expect(page.execute_script("return App.Map.maps[0].getZoom();")).to eq(11) + end + end end scenario "Skip map", :js do @@ -293,3 +327,11 @@ def set_arguments(arguments, mappable, mappable_path_arguments) arguments.merge!("#{argument_name}": mappable.send(path_to_value)) end end + +def map_zoom_in + initial_zoom = page.execute_script("return App.Map.maps[0].getZoom();") + find(".leaflet-control-zoom-in").click + until page.execute_script("return App.Map.maps[0].getZoom() === #{initial_zoom + 1};") do + sleep 0.01 + end +end From b9ce68bc821bc2dba1cb8270d875f2e14a21d81b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Tue, 11 Aug 2020 13:02:32 +0200 Subject: [PATCH 15/15] Set marker coordinates as map center when map location fields has valid coordinates When a user recovers a page from browser history where placed a marker in different map pane (visible map layer) marker was successfully added to the map but the map center is the one defined at Settings map properties so the marker was not visible to the user. Now when map_location form has valid coordinates we use them instead of default map center settings. This will avoid the user to have to rellocate the marker (or find the correct pane where the marker was added) if already placed. --- app/assets/javascripts/map.js | 21 +++++++++++++----- spec/shared/system/mappable.rb | 40 ++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index 0dc8d15d3..dc9c92ef1 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -20,14 +20,12 @@ App.Map.maps = []; }, initializeMap: function(element) { - var addMarkerInvestments, clearFormfields, createMarker, editable, formCoordinates, getPopupContent, - latitudeInputSelector, longitudeInputSelector, map, mapAttribution, mapCenterLatLng, + var addMarkerInvestments, clearFormfields, createMarker, dataCoordinates, editable, formCoordinates, + getPopupContent, latitudeInputSelector, longitudeInputSelector, map, mapAttribution, mapCenterLatLng, mapCenterLatitude, mapCenterLongitude, mapTilesProvider, marker, markerIcon, markerLatitude, markerLongitude, moveOrPlaceMarker, openMarkerPopup, removeMarker, removeMarkerSelector, updateFormfields, zoom, zoomInputSelector; App.Map.cleanInvestmentCoordinates(element); - mapCenterLatitude = $(element).data("map-center-latitude"); - mapCenterLongitude = $(element).data("map-center-longitude"); mapTilesProvider = $(element).data("map-tiles-provider"); mapAttribution = $(element).data("map-tiles-provider-attribution"); latitudeInputSelector = $(element).data("latitude-input-selector"); @@ -38,12 +36,23 @@ long: $(longitudeInputSelector).val(), zoom: $(zoomInputSelector).val() }; + dataCoordinates = { + lat: $(element).data("marker-latitude"), + long: $(element).data("marker-longitude") + }; if (App.Map.validCoordinates(formCoordinates)) { markerLatitude = formCoordinates.lat; markerLongitude = formCoordinates.long; + mapCenterLatitude = formCoordinates.lat; + mapCenterLongitude = formCoordinates.long; + } else if (App.Map.validCoordinates(dataCoordinates)) { + markerLatitude = dataCoordinates.lat; + markerLongitude = dataCoordinates.long; + mapCenterLatitude = dataCoordinates.lat; + mapCenterLongitude = dataCoordinates.lat; } else { - markerLatitude = $(element).data("marker-latitude"); - markerLongitude = $(element).data("marker-longitude"); + mapCenterLatitude = $(element).data("map-center-latitude"); + mapCenterLongitude = $(element).data("map-center-longitude"); } if (App.Map.validZoom(formCoordinates.zoom)) { zoom = formCoordinates.zoom; diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index 8d1df1aab..70c4266d7 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -134,6 +134,38 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, expect(page.execute_script("return App.Map.maps[0].getZoom();")).to eq(11) end end + + scenario "shows marker at map center", :js do + do_login_for user + visit send(mappable_new_path, arguments) + + within ".map_location" do + expect(page).not_to have_css(".map-icon") + end + + place_map_at(-68.592487, -62.391357) + find("#new_map_location").click + + within ".map_location" do + expect(page).to have_css(".map-icon") + end + + if management + click_link "Select user" + + expect(page).to have_content "User management" + else + click_link "Help" + + expect(page).to have_content "CONSUL is a platform for citizen participation" + end + + go_back + + within ".map_location" do + expect(page).to have_css(".map-icon") + end + end end scenario "Skip map", :js do @@ -335,3 +367,11 @@ def map_zoom_in sleep 0.01 end end + +def place_map_at(latitude, longitude) + page.execute_script("App.Map.maps[0].setView(new L.LatLng(#{latitude}, #{longitude}))") + + until page.execute_script("return App.Map.maps[0].getCenter().lat === #{latitude};") do + sleep 0.01 + end +end