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] 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