Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not reload page after adding comments in Pull Request reviews #13877

Merged
merged 9 commits into from
Jan 8, 2021

Conversation

jpraet
Copy link
Member

@jpraet jpraet commented Dec 6, 2020

Fixes #8861.

Use ajax on PR review page for submitting review comments, adding replies and (un)resolving conversations, instead of refreshing the entire page on each interaction.

@codecov-io
Copy link

codecov-io commented Dec 6, 2020

Codecov Report

Merging #13877 (a8becbe) into master (3c96a37) will decrease coverage by 0.03%.
The diff coverage is 26.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13877      +/-   ##
==========================================
- Coverage   41.85%   41.81%   -0.04%     
==========================================
  Files         743      743              
  Lines       79393    79453      +60     
==========================================
- Hits        33228    33223       -5     
- Misses      40703    40766      +63     
- Partials     5462     5464       +2     
Impacted Files Coverage Δ
modules/auth/repo_form.go 39.83% <ø> (ø)
routers/repo/pull_review.go 0.00% <0.00%> (ø)
models/issue_comment.go 52.28% <62.06%> (-0.44%) ⬇️
routers/routes/macaron.go 93.18% <100.00%> (+<0.01%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/queue/manager.go 62.13% <0.00%> (-2.96%) ⬇️
modules/charset/charset.go 68.53% <0.00%> (-2.25%) ⬇️
services/gitdiff/gitdiff.go 68.99% <0.00%> (-1.94%) ⬇️
routers/api/v1/repo/pull.go 24.84% <0.00%> (-0.61%) ⬇️
services/pull/pull.go 42.57% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c96a37...a8becbe. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 6, 2020
@lafriks lafriks added the topic/ui Change the appearance of the Gitea UI label Dec 7, 2020
@lafriks lafriks added this to the 1.14.0 milestone Dec 7, 2020
@silverwind
Copy link
Member

silverwind commented Dec 9, 2020

Some suggestions. Mostly async ajax requests and simplification.
diff --git a/web_src/js/index.js b/web_src/js/index.js
index fbbd97bdf..b94bf527c 100644
--- a/web_src/js/index.js
+++ b/web_src/js/index.js
@@ -1275,9 +1275,9 @@ function initPullRequestReview() {
     e.preventDefault();
     $(this).closest('.menu').toggle('visible');
   });

-  $('a.add-code-comment').on('click', function (e) {
+  $('a.add-code-comment').on('click', async function (e) {
     if ($(e.target).hasClass('btn-add-single')) return; // https://github.com/go-gitea/gitea/issues/4745
     e.preventDefault();

     const isSplit = $(this).closest('.code-diff').hasClass('code-diff-split');
@@ -1308,22 +1308,20 @@ function initPullRequestReview() {

     const td = ntr.find(`.add-comment-${side}`);
     let commentCloud = td.find('.comment-code-cloud');
     if (commentCloud.length === 0 && !ntr.find('button[name="is_review"]').length) {
-      const url = $(this).data('new-comment-url');
-      $.get(url, (data) => {
-        td.html(data);
-        commentCloud = td.find('.comment-code-cloud');
-        assingMenuAttributes(commentCloud.find('.menu'));
-        td.find("input[name='line']").val(idx);
-        td.find("input[name='side']").val(side === 'left' ? 'previous' : 'proposed');
-        td.find("input[name='path']").val(path);
-        const $textarea = commentCloud.find('textarea');
-        attachTribute($textarea.get(), {mentions: true, emoji: true});
-        const $simplemde = setCommentSimpleMDE($textarea);
-        $textarea.focus();
-        $simplemde.codemirror.focus();
-      });
+      const data = await $.get($(this).data('new-comment-url'));
+      td.html(data);
+      commentCloud = td.find('.comment-code-cloud');
+      assingMenuAttributes(commentCloud.find('.menu'));
+      td.find("input[name='line']").val(idx);
+      td.find("input[name='side']").val(side === 'left' ? 'previous' : 'proposed');
+      td.find("input[name='path']").val(path);
+      const $textarea = commentCloud.find('textarea');
+      attachTribute($textarea.get(), {mentions: true, emoji: true});
+      const $simplemde = setCommentSimpleMDE($textarea);
+      $textarea.focus();
+      $simplemde.codemirror.focus();
     }
   });
 }

@@ -2483,31 +2481,26 @@ $(document).ready(async () => {
     e.checked = false;
     $(e).trigger('click');
   });

-  $(document).on('click', '.resolve-conversation', function (e) {
+  $(document).on('click', '.resolve-conversation', async function (e) {
     e.preventDefault();
-    const id = $(this).data('comment-id');
+    const comment_id = $(this).data('comment-id');
     const origin = $(this).data('origin');
     const action = $(this).data('action');
     const url = $(this).data('update-url');

-    $.post(url, {
-      _csrf: csrf,
-      origin,
-      action,
-      comment_id: id,
-    }, (data) => {
-      if ($(this).closest('.conversation-holder').length) {
-        const conversation = $(data);
-        $(this).closest('.conversation-holder').replaceWith(conversation);
-        conversation.find('.dropdown').dropdown();
-        initReactionSelector(conversation);
-        initClipboard();
-      } else {
-        reload();
-      }
-    });
+    const data = await $.post(url, {_csrf: csrf, origin, action, comment_id});

+    if ($(this).closest('.conversation-holder').length) {
+      const conversation = $(data);
+      $(this).closest('.conversation-holder').replaceWith(conversation);
+      conversation.find('.dropdown').dropdown();
+      initReactionSelector(conversation);
+      initClipboard();
+    } else {
+      reload();
+    }
   });

   buttonsClickOnEnter();
   searchUsers();
@@ -3617,27 +3610,23 @@ function initIssueList() {
 $(document).on('click', 'button[name="is_review"]', (e) => {
   $(e.target).closest('form').append('<input type="hidden" name="is_review" value="true">');
 });

-$(document).on('submit', '.conversation-holder form', (e) => {
+$(document).on('submit', '.conversation-holder form', async (e) => {
   e.preventDefault();
   const form = $(e.target);
-  $.post(form.attr('action'), form.serialize(), (data) => {
-    const conversation = $(data);
-    const path = conversation.data('path');
-    const side = conversation.data('side');
-    const idx = conversation.data('idx');
-    const lineType = form.closest('tr').data('line-type');
-    form.closest('.conversation-holder').replaceWith(conversation);
-    if (lineType === 'same') {
-      $(`a.add-code-comment[data-path="${path}"][data-idx="${idx}"]`).addClass('invisible');
-    } else {
-      $(`a.add-code-comment[data-path="${path}"][data-side="${side}"][data-idx="${idx}"]`).addClass('invisible');
-    }
-    conversation.find('.dropdown').dropdown();
-    initReactionSelector(conversation);
-    initClipboard();
-  });
+  const newConversationHolder = $(await $.post(form.attr('action'), form.serialize()));
+  const {path, side, idx} = newConversationHolder.data();

+  form.closest('.conversation-holder').replaceWith(newConversationHolder);
+  if (form.closest('tr').data('line-type') === 'same') {
+    $(`a.add-code-comment[data-path="${path}"][data-idx="${idx}"]`).addClass('invisible');
+  } else {
+    $(`a.add-code-comment[data-path="${path}"][data-side="${side}"][data-idx="${idx}"]`).addClass('invisible');
+  }
+  newConversationHolder.find('.dropdown').dropdown();
+  initReactionSelector(newConversationHolder);
+  initClipboard();
 });

 window.cancelCodeComment = function (btn) {
   const form = $(btn).closest('form');

@jpraet jpraet marked this pull request as draft December 10, 2020 18:38
FetchCodeCommentsByLine was initially more or less copied from fetchCodeCommentsByReview. Now they both use a common findCodeComments function instead
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 10, 2020
@jpraet
Copy link
Member Author

jpraet commented Dec 10, 2020

Rebased because there were some merge conflicts. I also extracted some duplicate code. FetchCodeCommentsByLine was initially more or less copied from fetchCodeCommentsByReview. Now they both use a common findCodeComments function instead.

@jpraet jpraet marked this pull request as ready for review December 10, 2020 21:16
@jpraet
Copy link
Member Author

jpraet commented Jan 6, 2021

ping 👀

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 7, 2021
@lafriks
Copy link
Member

lafriks commented Jan 8, 2021

🚀

@lafriks lafriks changed the title #8861 use ajax on PR review page Do not reload page after adding comments in Pull Request reviews Jan 8, 2021
@lafriks lafriks merged commit bcb7f35 into go-gitea:master Jan 8, 2021
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Jan 8, 2021
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Jan 14, 2021
* master: (252 commits)
  Issues overview should not show issues from archived repos (go-gitea#13220)
  Display SVG files as images instead of text (go-gitea#14101)
  [skip ci] Updated translations via Crowdin
  Update docs to clarify issues raised in go-gitea#14272 (go-gitea#14318)
  [skip ci] Updated translations via Crowdin
  [Refactor] Passwort Hash/Set (go-gitea#14282)
  Add option to change username to the admin panel (go-gitea#14229)
  fix mailIssueCommentBatch for pull request (go-gitea#14252)
  Remove self from MAINTAINERS (go-gitea#14286)
  Do not reload page after adding comments in Pull Request reviews (go-gitea#13877)
  Fix session bug when introduce chi (go-gitea#14287)
  [skip ci] Updated translations via Crowdin
  Add secure/httpOnly attributes to the lang cookie (go-gitea#9690) (go-gitea#14279)
  Some code improvements (go-gitea#14266)
  [skip ci] Updated translations via Crowdin
  Fix wrong type on hooktask to convert typ from char(16) to varchar(16) (go-gitea#14148)
  Upgrade XORM links in documentation. (go-gitea#14265)
  Check permission for the appropriate unit type (go-gitea#14261)
  Add compliance check for windows to ensure cross platform build (go-gitea#14260)
  [skip ci] Updated translations via Crowdin
  ...
@jpraet jpraet deleted the 8861-review-ajax branch January 22, 2021 18:38
@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review page should not get refresh on adding comment
9 participants