From 24b7cade490d0ef6d208ec6a524251e0f82edc66 Mon Sep 17 00:00:00 2001
From: Naoki Takezoe <takezoe@gmail.com>
Date: Sat, 25 Nov 2023 23:48:48 +0900
Subject: [PATCH] Simplify pull request UI (#3417)

---
 .../gitbucket/core/pulls/compare.scala.html   |  15 +--
 .../core/pulls/mergeguide.scala.html          | 121 +++++++++---------
 .../gitbucket/core/repo/branches.scala.html   |  18 +--
 3 files changed, 69 insertions(+), 85 deletions(-)

diff --git a/src/main/twirl/gitbucket/core/pulls/compare.scala.html b/src/main/twirl/gitbucket/core/pulls/compare.scala.html
index 6f0e22bdf..df896a14f 100644
--- a/src/main/twirl/gitbucket/core/pulls/compare.scala.html
+++ b/src/main/twirl/gitbucket/core/pulls/compare.scala.html
@@ -58,12 +58,7 @@
       }
     </div>
     @if(commits.nonEmpty && context.loginAccount.isDefined && originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId)){
-      <div style="margin-bottom: 10px; padding: 8px; background-color: #fff9ea" id="create-pull-request" class="box-content">
-        <a href="#" class="btn btn-success" id="show-form">Create pull request</a>
-        &nbsp;&nbsp;
-        <span class="muted">Discuss and review the changes in this comparison with others.</span>
-      </div>
-      <div id="pull-request-form" style="display: none; margin-bottom: 20px;">
+      <div id="pull-request-form" style="margin-bottom: 20px;">
         <form method="POST" action="@context.path/@originRepository.owner/@originRepository.name/pulls/new" validate="true">
           <div class="row">
             <div class="col-md-9">
@@ -239,14 +234,6 @@ $(function(){
       $.trim($('i.octicon-check').parents('a.forked-branch').data('branch'));
   });
 
-  $('#show-form').click(function(){
-    $('#create-pull-request').hide();
-    $('#pull-request-form').show();
-  });
-  if(location.search.substr(1).split("&").indexOf("expand=1")!=-1){
-    $('#show-form').click();
-  }
-
   @if(context.loginAccount.isDefined && originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId)){
     function checkConflict(from, to){
       $('.check-conflict').show();
diff --git a/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html b/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html
index 3b4064238..5c889d67b 100644
--- a/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html
+++ b/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html
@@ -77,35 +77,68 @@
           } else {
             <div class="merge-indicator merge-indicator-success"><span class="octicon octicon-check"></span></div>
 
-        @if(pullreq.isDraft){
-          <span class="strong">This pull request is still a work in progress.</span>
-          <div class="pull-right">
-            <input type="button" class="btn btn-default" value="Ready for review" id="ready-for-review" />
-          </div>
-          <div class="small">
-            Draft pull requests cannot be merged.
-          </div>
-        } else {
-            @if(status.hasMergePermission){
-              <span class="strong">Merging can be performed automatically.</span>
+            @if(pullreq.isDraft){
+              <span class="strong">This pull request is still a work in progress.</span>
+              <div class="pull-right">
+                <input type="button" class="btn btn-default" value="Ready for review" id="ready-for-review" />
+              </div>
               <div class="small">
-                Merging can be performed automatically.
+                Draft pull requests cannot be merged.
               </div>
             } else {
-              <span class="strong">This branch has no conflicts with the base branch.</span>
-              <div class="small">
-                Only those with write access to this repository can merge pull requests.
-              </div>
+              @if(status.hasMergePermission){
+                <span class="strong">Merging can be performed automatically.</span>
+                <div class="small">
+                  Merging can be performed automatically.
+                </div>
+              } else {
+                <span class="strong">This branch has no conflicts with the base branch.</span>
+                <div class="small">
+                  Only those with write access to this repository can merge pull requests.
+                </div>
+              }
             }
-           }
           }
         }
       }
       </div>
       @if(status.hasMergePermission){
         <div style="padding:15px; border-top:solid 1px #e5e5e5; background:#fafafa">
-          <input type="button" class="btn @if(!status.hasProblem){btn-success} else {btn-default}" id="merge-pull-request-button" value="Merge pull request"@if(!status.canMerge || pullreq.isDraft){ disabled="true"}/>
-          &nbsp;&nbsp;You can also merge branches on the <a href="#" class="show-command-line">command line</a>.
+          <div class="btn-group">
+            <input type="button" class="btn @if(!status.hasProblem){btn-success} else {btn-default}" id="merge-pull-request-button"
+                   value="@mergeStrategyLabel(originRepository.repository.options.defaultMergeOption)"
+                   @if(!status.canMerge || pullreq.isDraft){disabled="true"}/>
+            <button id="merge-strategy-btn" class="dropdown-toggle btn @if(!status.hasProblem){btn-success} else {btn-default}" data-toggle="dropdown"
+                    @if(!status.canMerge || pullreq.isDraft){ disabled="true"}>
+              <span class="caret"></span>
+            </button>
+            <ul class="dropdown-menu">
+            @defining(originRepository.repository.options.mergeOptions.split(",")){ mergeOptions =>
+              @if(mergeOptions.contains("merge-commit")){
+                <li>
+                  <a href="javascript:void(0);" class="merge-strategy" data-value="merge-commit">
+                    <strong>@mergeStrategyLabel("merge-commit")</strong><br>These commits will be added to the base branch via a merge commit.
+                  </a>
+                </li>
+              }
+              @if(mergeOptions.contains("squash")){
+                <li>
+                  <a href="javascript:void(0);" class="merge-strategy" data-value="squash">
+                    <strong>@mergeStrategyLabel("squash")</strong><br>These commits will be combined into one commit in the base branch.
+                  </a>
+                </li>
+              }
+              @if(mergeOptions.contains("rebase")){
+                <li>
+                  <a href="javascript:void(0);" class="merge-strategy" data-value="rebase">
+                    <strong>@mergeStrategyLabel("rebase")</strong><br>These commits will be rebased and added to the base branch.
+                  </a>
+                </li>
+              }
+            }
+            </ul>
+          </div>
+        &nbsp;&nbsp;You can also merge branches on the <a href="#" class="show-command-line">command line</a>.
           <div id="command-line" style="display: none;margin-top: 15px;">
             <hr />
             @if(status.hasConflict){
@@ -161,44 +194,7 @@
         <span id="error-message" class="error"></span>
         <textarea name="message" style="height: 80px; margin-top: 8px; margin-bottom: 8px;" class="form-control">@issue.title</textarea>
         <div>
-          <div class="btn-group">
-            <button id="merge-strategy-btn" class="dropdown-toggle btn btn-default" data-toggle="dropdown">
-              <span class="strong">
-                @(Map(
-                  "merge-commit" -> "Merge commit",
-                  "squash"       -> "Squash",
-                  "rebase"       -> "Rebase"
-                )(originRepository.repository.options.defaultMergeOption))
-              </span>
-              <span class="caret"></span>
-            </button>
-            <ul class="dropdown-menu">
-              @defining(originRepository.repository.options.mergeOptions.split(",")){ mergeOptions =>
-                @if(mergeOptions.contains("merge-commit")){
-                  <li>
-                    <a href="javascript:void(0);" class="merge-strategy" data-value="merge-commit">
-                      <strong>Merge commit</strong><br>These commits will be added to the base branch via a merge commit.
-                    </a>
-                  </li>
-                }
-                @if(mergeOptions.contains("squash")){
-                  <li>
-                    <a href="javascript:void(0);" class="merge-strategy" data-value="squash">
-                      <strong>Squash</strong><br>These commits will be combined into one commit in the base branch.
-                    </a>
-                  </li>
-                }
-                @if(mergeOptions.contains("rebase")){
-                  <li>
-                    <a href="javascript:void(0);" class="merge-strategy" data-value="rebase">
-                      <strong>Rebase</strong><br>These commits will be rebased and added to the base branch.
-                    </a>
-                  </li>
-                }
-              }
-            </ul>
-          </div>
-          <div class="pull-right">
+          <div>
             <input type="button" class="btn btn-default" value="Cancel" id="cancel-merge-pull-request"/>
             <input type="submit" class="btn btn-success" value="Confirm merge"/>
             <input type="hidden" name="strategy" value="@originRepository.repository.options.defaultMergeOption"/>
@@ -268,8 +264,17 @@ $(function(){
   }
 
   $('.merge-strategy').click(function(){
-    $('button#merge-strategy-btn > span.strong').text($(this).find('strong').text());
+    $('input#merge-pull-request-button').val($(this).find('strong').text());
     $('input[name=strategy]').val($(this).data('value'));
+
   });
 });
 </script>
+
+@mergeStrategyLabel(value: String): String = @{
+  Map(
+    "merge-commit" -> "Create a merge commit",
+    "squash"       -> "Squash and merge",
+    "rebase"       -> "Rebase and merge"
+  )(value)
+}
diff --git a/src/main/twirl/gitbucket/core/repo/branches.scala.html b/src/main/twirl/gitbucket/core/repo/branches.scala.html
index 2019f070f..2feb99c95 100644
--- a/src/main/twirl/gitbucket/core/repo/branches.scala.html
+++ b/src/main/twirl/gitbucket/core/repo/branches.scala.html
@@ -60,19 +60,11 @@
                       <a href="@helpers.url(repository)/pull/@issue.issueId" title="@issue.title" class="label label-success">Open</a>
                     }
                   }.getOrElse{
-                    @if(context.loginAccount.isDefined){
-                      <a href="@helpers.url(repository)/compare/@{repository.repository.parentUserName.map { parent =>
-                        helpers.urlEncode(parent) + ":" + helpers.encodeRefName(repository.repository.defaultBranch)
-                      }.getOrElse {
-                        helpers.encodeRefName(repository.repository.defaultBranch)
-                      }}...@{helpers.encodeRefName(branch.name)}?expand=1" class="btn btn-default btn-sm">New Pull request</a>
-                    } else {
-                      <a href="@helpers.url(repository)/compare/@{repository.repository.parentUserName.map { parent =>
-                        helpers.urlEncode(parent) + ":" + helpers.encodeRefName(repository.repository.defaultBranch)
-                      }.getOrElse {
-                        helpers.encodeRefName(repository.repository.defaultBranch)
-                      }}...@{helpers.encodeRefName(branch.name)}" class="btn btn-default btn-sm">Compare</a>
-                    }
+                    <a href="@helpers.url(repository)/compare/@{repository.repository.parentUserName.map { parent =>
+                      helpers.urlEncode(parent) + ":" + helpers.encodeRefName(repository.repository.defaultBranch)
+                    }.getOrElse {
+                      helpers.encodeRefName(repository.repository.defaultBranch)
+                    }}...@{helpers.encodeRefName(branch.name)}" class="btn btn-default btn-sm">@if(context.loginAccount.isDefined){Create pull request} else {Compare}</a>
                   }
                   @if(hasWritePermission){
                     <span style="margin-left: 8px;">