From 70a9fb735568a1028595c4c25238ff584b40fa8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Mon, 4 May 2020 11:56:02 +0200 Subject: [PATCH] Do not call initialize_modules after every ajax call We were calling initialize_modules after every ajax call only to apply the application javascript to new elements added though ajax calls, this is not always needed because some ajax calls do not add new elements to the user interface so there is no new elements to initialize. This technique was working fine in most cases but was causing different kind of problems at some pages where some elements where being unnecessarily reinitiliazed causing the execution of the element associated scripts as many times as the element was initialized. This fix should relief a little bit of work to users browsers. After this change we had to fix some pieces of javascript: Regarding LegistationAnnotatable module, since we are not re-initializing Annotator app we cannot destroy it so now we only need to insert the new annotation into annotator interface to keep it properly updated. Regarding Comments module, we do not need anymore the initialization check because this code only will be fired once now we do not launch application initialization after ajax calls. Also, when a new comment is created its added to the DOM through AJAX and include some elements that needs javascript initialization to work. By using "Delegated event handlers" [1] new comments will be initialized automatically when added. [1] https://api.jquery.com/on/#direct-and-delegated-events --- app/assets/javascripts/application.js | 1 - app/assets/javascripts/comments.js | 27 +++++------ .../javascripts/legislation_annotatable.js | 2 +- spec/system/comments/debates_spec.rb | 45 +++++++++++++++++++ 4 files changed, 57 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 1acb57d86..cfdf34113 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -173,5 +173,4 @@ $(function() { $(document).ready(initialize_modules); $(document).on("page:load", initialize_modules); - $(document).on("ajax:complete", initialize_modules); }); diff --git a/app/assets/javascripts/comments.js b/app/assets/javascripts/comments.js index a7e85bbf8..e8127b705 100644 --- a/app/assets/javascripts/comments.js +++ b/app/assets/javascripts/comments.js @@ -43,23 +43,18 @@ $("span#" + id + "_arrow").toggleClass("fa-minus-square").toggleClass("fa-plus-square"); }, initialize: function() { - $("body .js-add-comment-link").each(function() { - if ($(this).data("initialized") !== "yes") { - $(this).on("click", function() { - App.Comments.toggle_form($(this).data().id); - return false; - }).data("initialized", "yes"); - } + $("body").on("click", ".js-add-comment-link", function() { + App.Comments.toggle_form($(this).data().id); + return false; }); - $("body .js-toggle-children").each(function() { - $(this).on("click", function() { - var children_container_id; - children_container_id = ($(this).data().id) + "_children"; - $("#" + children_container_id).toggle("slow"); - App.Comments.toggle_arrow(children_container_id); - $(this).children(".js-child-toggle").toggle(); - return false; - }); + + $("body").on("click", ".js-toggle-children", function() { + var children_container_id; + children_container_id = ($(this).data().id) + "_children"; + $("#" + children_container_id).toggle("slow"); + App.Comments.toggle_arrow(children_container_id); + $(this).children(".js-child-toggle").toggle(); + return false; }); } }; diff --git a/app/assets/javascripts/legislation_annotatable.js b/app/assets/javascripts/legislation_annotatable.js index ab5f5cc6d..c4286fa1a 100644 --- a/app/assets/javascripts/legislation_annotatable.js +++ b/app/assets/javascripts/legislation_annotatable.js @@ -108,9 +108,9 @@ App.LegislationAnnotatable.highlight("#7fff9a"); $("#comments-box textarea").focus(); $("#new_legislation_annotation").on("ajax:complete", function(e, data) { - App.LegislationAnnotatable.app.destroy(); if (data.status === 200) { App.LegislationAnnotatable.remove_highlight(); + App.LegislationAnnotatable.app.annotations.runHook("annotationCreated", [data.responseJSON]); $("#comments-box").html("").hide(); $.ajax({ method: "GET", diff --git a/spec/system/comments/debates_spec.rb b/spec/system/comments/debates_spec.rb index 731beda03..1b5db3283 100644 --- a/spec/system/comments/debates_spec.rb +++ b/spec/system/comments/debates_spec.rb @@ -84,6 +84,26 @@ describe "Commenting debates" do expect(page).not_to have_content grandchild_comment.body end + scenario "can collapse comments after adding a reply", :js do + parent_comment = create(:comment, body: "Main comment", commentable: debate) + create(:comment, body: "First subcomment", commentable: debate, parent: parent_comment) + + login_as(user) + visit debate_path(debate) + + within ".comment", text: "Main comment" do + first(:link, "Reply").click + fill_in "Leave your comment", with: "It will be done next week." + click_button "Publish reply" + + expect(page).to have_content("It will be done next week.") + + find(".fa-minus-square").click + + expect(page).not_to have_content("It will be done next week.") + end + end + scenario "Comment order" do c1 = create(:comment, :with_confidence_score, commentable: debate, cached_votes_up: 100, cached_votes_total: 120, created_at: Time.current - 2) @@ -230,6 +250,31 @@ describe "Commenting debates" do expect(page).not_to have_selector("#js-comment-form-comment_#{comment.id}", visible: true) end + scenario "Reply to reply", :js do + create(:comment, commentable: debate, body: "Any estimates?") + + login_as(create(:user)) + visit debate_path(debate) + + within ".comment", text: "Any estimates?" do + click_link "Reply" + fill_in "Leave your comment", with: "It will be done next week." + click_button "Publish reply" + end + + within ".comment .comment", text: "It will be done next week" do + click_link "Reply" + fill_in "Leave your comment", with: "Probably if government approves." + click_button "Publish reply" + + expect(page).not_to have_selector("form") + + within ".comment" do + expect(page).to have_content "Probably if government approves." + end + end + end + scenario "Errors on reply", :js do comment = create(:comment, commentable: debate, user: user)