Diff improvements (#23553)

- Avoid flash of wrong tree toggle icon on page load by setting icon
based on sync state
- Avoid "pop-in" of tree on page load by leaving space based on sync
state
- Use the same border/box-shadow combo used on comment `:target` also
for file `:target`.
- Refactor `DiffFileTree.vue` to use `toggleElem` instead of hardcoded
class name.
- Left-align inline comment boxes and make them fit the same amount of
markup content on a line as GitHub.
- Fix height of `diff-file-list`

Fixes: #23593

<img width="1250" alt="Screenshot 2023-03-18 at 00 52 04"
src="https://user-images.githubusercontent.com/115237/226071392-6789a644-aead-4756-a77e-aba3642150a0.png">
<img width="1246" alt="Screenshot 2023-03-18 at 00 59 43"
src="https://user-images.githubusercontent.com/115237/226071443-8bcba924-458b-48bd-b2f0-0de59cb180ac.png">
<img width="1250" alt="Screenshot 2023-03-18 at 01 27 14"
src="https://user-images.githubusercontent.com/115237/226073121-ccb99f9a-d3ac-40b7-9589-43580c4a01c9.png">
<img width="1231" alt="Screenshot 2023-03-19 at 21 44 16"
src="https://user-images.githubusercontent.com/115237/226207951-81bcae1b-6b41-4e39-83a7-0f37951df6be.png">

(Yes I'm aware the border-radius in bottom corners is suboptimal, but
this would be notorously hard to fix without relying on `overflow:
hidden`).

Author: silverwind

options/locale/locale_en-US.ini

@@ -2280,6 +2280,8 @@ diff.image.side_by_side = Side by Side
 diff.image.swipe = Swipe
 diff.image.overlay = Overlay
 diff.has_escaped = This line has hidden Unicode characters
+diff.show_file_tree = Show file tree
+diff.hide_file_tree = Hide file tree
 
 releases.desc = Track project versions and downloads.
 release.releases = Releases

templates/repo/diff/box.tmpl

@@ -15,11 +15,18 @@
 	<div>
 		<div class="diff-detail-box diff-box sticky gt-df gt-sb gt-ac gt-fw">
 			<div class="gt-df gt-ac gt-fw">
-				<a class="diff-toggle-file-tree-button muted gt-df gt-ac">
+				<button class="diff-toggle-file-tree-button gt-df gt-ac" data-show-text="{{.locale.Tr "repo.diff.show_file_tree"}}" data-hide-text="{{.locale.Tr "repo.diff.hide_file_tree"}}">
 					{{/* the icon meaning is reversed here, "octicon-sidebar-collapse" means show the file tree */}}
-					{{svg "octicon-sidebar-collapse" 20 "icon hide"}}
-					{{svg "octicon-sidebar-expand" 20 "icon"}}
-				</a>
+					{{svg "octicon-sidebar-collapse" 20 "icon gt-hidden"}}
+					{{svg "octicon-sidebar-expand" 20 "icon gt-hidden"}}
+				</button>
+				<script>
+					const diffTreeVisible = localStorage?.getItem('diff_file_tree_visible') === 'true';
+					const diffTreeBtn = document.querySelector('.diff-toggle-file-tree-button');
+					const diffTreeIcon = `.octicon-sidebar-${diffTreeVisible ? 'expand' : 'collapse'}`;
+					diffTreeBtn.querySelector(diffTreeIcon).classList.remove('gt-hidden');
+					diffTreeBtn.setAttribute('data-tooltip-content', diffTreeBtn.getAttribute(diffTreeVisible ? 'data-hide-text' : 'data-show-text'));
+				</script>
 				<div class="diff-detail-stats gt-df gt-ac gt-ml-3">
 					{{svg "octicon-diff" 16 "gt-mr-2"}}{{.locale.Tr "repo.diff.stats_desc" .Diff.NumFiles .Diff.TotalAddition .Diff.TotalDeletion | Str2html}}
 				</div>
@@ -68,6 +75,9 @@
 		<div id="diff-file-list"></div>
 		<div id="diff-container">
 				<div id="diff-file-tree" class="gt-hidden"></div>
+				<script>
+					if (diffTreeVisible) document.getElementById('diff-file-tree').classList.remove('gt-hidden');
+				</script>
 				<div id="diff-file-boxes" class="sixteen wide column">
 					{{range $i, $file := .Diff.Files}}
 						{{/*notice: the index of Diff.Files should not be used for element ID, because the index will be restarted from 0 when doing load-more for PRs with a lot of files*/}}

web_src/css/base.css

@@ -238,6 +238,10 @@ table {
   border-collapse: collapse;
 }
 
+button {
+  cursor: pointer;
+}
+
 details summary {
   cursor: pointer;
 }

web_src/css/repository.css

@@ -1616,14 +1616,12 @@
   padding: 7px 0;
   background: var(--color-body);
   line-height: 30px;
-  height: 47px; /* match .ui.attached.header.diff-file-header.sticky-2nd-row */
 }
 
 @media (max-width: 991px) {
   .repository .diff-detail-box {
     flex-direction: column;
     align-items: flex-start;
-    height: 77px; /* match .ui.attached.header.diff-file-header.sticky-2nd-row */
   }
 }
 
@@ -1679,6 +1677,13 @@
   }
 }
 
+.diff-detail-actions {
+  /* prevent font-size from increasing element height so that .diff-detail-box comes
+     out with height of 47px (one line) and 77px (two lines), which is important for
+     position: sticky */
+  height: 33px;
+}
+
 .repository .diff-detail-box .diff-detail-actions > * {
   margin-right: 0;
 }
@@ -1853,10 +1858,24 @@
   padding-bottom: 5px;
 }
 
+.diff-file-box {
+  border: 1px solid transparent;
+  border-radius: var(--border-radius);
+}
+
+/* TODO: this can potentially be made "global" by removing the class prefix */
+.diff-file-box .ui.attached.header,
+.diff-file-box .ui.attached.table {
+  margin: 0; /* remove fomantic negative margins */;
+  width: initial; /* remove fomantic over 100% width */;
+  max-width: initial; /* remove fomantic over 100% width */;
+}
+
 .repository .diff-stats {
   clear: both;
   margin-bottom: 5px;
-  max-height: 400px;
+  max-height: 200px;
+  height: fit-content;
   overflow: auto;
   padding-left: 0;
 }
@@ -2652,7 +2671,8 @@
   filter: drop-shadow(-3px 0 0 var(--color-primary-alpha-30)) !important;
 }
 
-.code-comment:target {
+.code-comment:target,
+.diff-file-box:target {
   border-color: var(--color-primary) !important;
   border-radius: var(--border-radius) !important;
   box-shadow: 0 0 0 3px var(--color-primary-alpha-30) !important;
@@ -3226,17 +3246,28 @@ td.blob-excerpt {
 }
 
 #diff-file-tree {
-  width: 20%;
+  flex: 0 0 20%;
   max-width: 380px;
   line-height: inherit;
   position: sticky;
   padding-top: 0;
   top: 47px;
-  max-height: calc(100vh - 50px);
+  max-height: calc(100vh - 47px);
   height: 100%;
   overflow-y: auto;
 }
 
+.diff-toggle-file-tree-button {
+  background: none;
+  border: none;
+  user-select: none;
+  color: inherit;
+}
+
+.diff-toggle-file-tree-button:hover {
+  color: var(--color-primary);
+}
+
 @media (max-width: 991px) {
   #diff-file-tree {
     display: none !important;

web_src/css/review.css

@@ -67,8 +67,7 @@
 .comment-code-cloud {
   padding: 0.5rem 1rem !important;
   position: relative;
-  margin: 0 auto;
-  max-width: 1000px;
+  max-width: 820px;
 }
 
 @media (max-width: 767px) {
@@ -308,11 +307,3 @@ a.blob-excerpt:hover {
   width: 72px;
   height: 10px;
 }
-
-.diff-file-box {
-  border-radius: 0.285rem; /* Just like ui.top.attached.header */
-}
-
-.diff-file-box:target {
-  box-shadow: 0 0 0 3px var(--color-accent);
-}

web_src/js/components/DiffFileTree.vue

@@ -16,6 +16,7 @@
 <script>
 import DiffFileTreeItem from './DiffFileTreeItem.vue';
 import {doLoadMoreFiles} from '../features/repo-diff.js';
+import {toggleElem} from '../utils/dom.js';
 
 const {pageData} = window.config;
 const LOCAL_STORAGE_KEY = 'diff_file_tree_visible';
@@ -92,8 +93,6 @@ export default {
     }
   },
   mounted() {
-    // ensure correct buttons when we are mounted to the dom
-    this.adjustToggleButton(this.fileTreeIsVisible);
     // replace the pageData.diffFileInfo.files with our watched data so we get updates
     pageData.diffFileInfo.files = this.files;
 
@@ -109,15 +108,17 @@ export default {
     updateVisibility(visible) {
       this.fileTreeIsVisible = visible;
       localStorage.setItem(LOCAL_STORAGE_KEY, this.fileTreeIsVisible);
-      this.adjustToggleButton(this.fileTreeIsVisible);
+      this.updateState(this.fileTreeIsVisible);
     },
-    adjustToggleButton(visible) {
-      const [toShow, toHide] = document.querySelectorAll('.diff-toggle-file-tree-button .icon');
-      toShow.classList.toggle('gt-hidden', visible);  // hide the toShow icon if the tree is visible
-      toHide.classList.toggle('gt-hidden', !visible); // similarly
-
-      const diffTree = document.getElementById('diff-file-tree');
-      diffTree.classList.toggle('gt-hidden', !visible);
+    updateState(visible) {
+      const btn = document.querySelector('.diff-toggle-file-tree-button');
+      const [toShow, toHide] = btn.querySelectorAll('.icon');
+      const tree = document.getElementById('diff-file-tree');
+      const newTooltip = btn.getAttribute(visible ? 'data-hide-text' : 'data-show-text');
+      btn.setAttribute('data-tooltip-content', newTooltip);
+      toggleElem(tree, visible);
+      toggleElem(toShow, !visible);
+      toggleElem(toHide, visible);
     },
     loadMoreData() {
       this.isLoadingNewData = true;

web_src/js/components/DiffFileTreeItem.vue

@@ -1,7 +1,7 @@
 <template>
   <div v-show="show" :title="item.name">
     <!--title instead of tooltip above as the tooltip needs too much work with the current methods, i.e. not being loaded or staying open for "too long"-->
-    <div class="item" :class="item.isFile ? 'filewrapper gt-p-1' : ''">
+    <div class="item" :class="item.isFile ? 'filewrapper gt-p-1 gt-ac' : ''">
       <!-- Files -->
       <SvgIcon
         v-if="item.isFile"
@@ -10,7 +10,7 @@
       />
       <a
         v-if="item.isFile"
-        class="file gt-ellipsis muted"
+        class="file gt-ellipsis"
         :href="item.isFile ? '#diff-' + item.file.NameHash : ''"
       >{{ item.name }}</a>
       <SvgIcon
@@ -20,7 +20,7 @@
       />
 
       <!-- Directories -->
-      <div v-if="!item.isFile" class="directory gt-p-1" @click.stop="handleClick(item.isFile)">
+      <div v-if="!item.isFile" class="directory gt-p-1 gt-ac" @click.stop="handleClick(item.isFile)">
         <SvgIcon
           class="svg-icon"
           :name="collapsed ? 'octicon-chevron-right' : 'octicon-chevron-down'"
@@ -79,31 +79,31 @@ export default {
 </script>
 
 <style scoped>
-span.svg-icon.status {
+.svg-icon.status {
   float: right;
 }
 
-span.svg-icon.file {
+.svg-icon.file {
   color: var(--color-secondary-dark-7);
 }
 
-span.svg-icon.directory {
+.svg-icon.directory {
   color: var(--color-primary);
 }
 
-span.svg-icon.octicon-diff-modified {
+.svg-icon.octicon-diff-modified {
   color: var(--color-yellow);
 }
 
-span.svg-icon.octicon-diff-added {
+.svg-icon.octicon-diff-added {
   color: var(--color-green);
 }
 
-span.svg-icon.octicon-diff-removed {
+.svg-icon.octicon-diff-removed {
   color: var(--color-red);
 }
 
-span.svg-icon.octicon-diff-renamed {
+.svg-icon.octicon-diff-renamed {
   color: var(--color-teal);
 }
 
@@ -139,9 +139,11 @@ div.list {
 
 a {
   text-decoration: none;
+  color: var(--color-text);
 }
 
 a:hover {
   text-decoration: none;
+  color: var(--color-text);
 }
 </style>