From 1d01e164ec7bbf0b30ba21753604227eaa469216 Mon Sep 17 00:00:00 2001
From: Sam Bigelow <sbigelow@gitlab.com>
Date: Thu, 28 Mar 2019 17:37:06 -0400
Subject: Improve diff navigation header

- Compare versions header is full width except in the unified diff mode
with no tree sidebar
  - Bar is always full width, but the content within stays centered when
unified and no tree sidebar
- File header is the same height as the "Compare versions header"
- aligns with the design system grid guidelines => 56px
- Diff file headers use a button group, switch icon order to open file
externally being the last option, all buttons will become icon buttons
(icon delivery by @dimitrieh)
- If a file header becomes sticky no rounded corner/double border
problem is visible anymore
---
 app/assets/javascripts/breakpoints.js              |   3 +
 app/assets/javascripts/diffs/components/app.vue    |  15 ++-
 .../diffs/components/compare_versions.vue          |  18 ++-
 .../diffs/components/diff_file_header.vue          | 137 +++++++++++----------
 app/assets/javascripts/diffs/constants.js          |   3 +
 app/assets/javascripts/lib/utils/common_utils.js   |   9 +-
 app/assets/stylesheets/framework/files.scss        |   4 +
 app/assets/stylesheets/pages/diff.scss             |   8 +-
 app/assets/stylesheets/pages/merge_requests.scss   |  16 +--
 .../unreleased/57364-improve-diff-nav-header.yml   |   5 +
 spec/javascripts/diffs/components/app_spec.js      |  18 +++
 .../diffs/components/compare_versions_spec.js      |  20 +++
 .../diffs/components/diff_file_header_spec.js      |   4 +-
 spec/javascripts/lib/utils/common_utils_spec.js    |   6 +-
 14 files changed, 179 insertions(+), 87 deletions(-)
 create mode 100644 changelogs/unreleased/57364-improve-diff-nav-header.yml

diff --git a/app/assets/javascripts/breakpoints.js b/app/assets/javascripts/breakpoints.js
index 7951348d8b2..4d5d6bb864b 100644
--- a/app/assets/javascripts/breakpoints.js
+++ b/app/assets/javascripts/breakpoints.js
@@ -14,6 +14,9 @@ const BreakpointInstance = {
 
     return breakpoint;
   },
+  isDesktop() {
+    return ['lg', 'md'].includes(this.getBreakpointSize);
+  },
 };
 
 export default BreakpointInstance;
diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index e8f8c09152a..5e74998579b 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -20,6 +20,7 @@ import {
   MAX_TREE_WIDTH,
   TREE_HIDE_STATS_WIDTH,
   MR_TREE_SHOW_KEY,
+  CENTERED_LIMITED_CONTAINER_CLASSES,
 } from '../constants';
 
 export default {
@@ -114,6 +115,9 @@ export default {
     hideFileStats() {
       return this.treeWidth <= TREE_HIDE_STATS_WIDTH;
     },
+    isLimitedContainer() {
+      return !this.showTreeList && !this.isParallelView;
+    },
   },
   watch: {
     diffViewType() {
@@ -148,6 +152,7 @@ export default {
     this.adjustView();
     eventHub.$once('fetchedNotesData', this.setDiscussions);
     eventHub.$once('fetchDiffData', this.fetchData);
+    this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES;
   },
   beforeDestroy() {
     eventHub.$off('fetchDiffData', this.fetchData);
@@ -202,8 +207,6 @@ export default {
     adjustView() {
       if (this.shouldShow) {
         this.$nextTick(() => {
-          window.mrTabs.resetViewContainer();
-          window.mrTabs.expandViewContainer(this.showTreeList);
           this.setEventListeners();
         });
       } else {
@@ -256,6 +259,7 @@ export default {
         :merge-request-diffs="mergeRequestDiffs"
         :merge-request-diff="mergeRequestDiff"
         :target-branch="targetBranch"
+        :is-limited-container="isLimitedContainer"
       />
 
       <hidden-files-warning
@@ -285,7 +289,12 @@ export default {
           />
           <tree-list :hide-file-stats="hideFileStats" />
         </div>
-        <div class="diff-files-holder">
+        <div
+          class="diff-files-holder"
+          :class="{
+            [CENTERED_LIMITED_CONTAINER_CLASSES]: isLimitedContainer,
+          }"
+        >
           <commit-widget v-if="commit" :commit="commit" />
           <template v-if="renderDiffFiles">
             <diff-file
diff --git a/app/assets/javascripts/diffs/components/compare_versions.vue b/app/assets/javascripts/diffs/components/compare_versions.vue
index fe49dfff10b..363ebad1594 100644
--- a/app/assets/javascripts/diffs/components/compare_versions.vue
+++ b/app/assets/javascripts/diffs/components/compare_versions.vue
@@ -7,6 +7,7 @@ import Icon from '~/vue_shared/components/icon.vue';
 import CompareVersionsDropdown from './compare_versions_dropdown.vue';
 import SettingsDropdown from './settings_dropdown.vue';
 import DiffStats from './diff_stats.vue';
+import { CENTERED_LIMITED_CONTAINER_CLASSES } from '../constants';
 
 export default {
   components: {
@@ -35,6 +36,11 @@ export default {
       required: false,
       default: null,
     },
+    isLimitedContainer: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   computed: {
     ...mapGetters('diffs', ['hasCollapsedFile', 'diffFilesLength']),
@@ -62,6 +68,9 @@ export default {
       return this.mergeRequestDiff.base_version_path;
     },
   },
+  created() {
+    this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES;
+  },
   mounted() {
     polyfillSticky(this.$el);
   },
@@ -77,8 +86,13 @@ export default {
 </script>
 
 <template>
-  <div class="mr-version-controls" :class="{ 'is-fileTreeOpen': showTreeList }">
-    <div class="mr-version-menus-container content-block">
+  <div class="mr-version-controls border-top border-bottom">
+    <div
+      class="mr-version-menus-container content-block"
+      :class="{
+        [CENTERED_LIMITED_CONTAINER_CLASSES]: isLimitedContainer,
+      }"
+    >
       <button
         v-gl-tooltip.hover
         type="button"
diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue
index fda7b7c5fd9..2136a61b1ae 100644
--- a/app/assets/javascripts/diffs/components/diff_file_header.vue
+++ b/app/assets/javascripts/diffs/components/diff_file_header.vue
@@ -1,7 +1,7 @@
 <script>
 import _ from 'underscore';
 import { mapActions, mapGetters } from 'vuex';
-import { polyfillSticky } from '~/lib/utils/sticky';
+import { polyfillSticky, stickyMonitor } from '~/lib/utils/sticky';
 import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
 import Icon from '~/vue_shared/components/icon.vue';
 import FileIcon from '~/vue_shared/components/file_icon.vue';
@@ -11,7 +11,7 @@ import { __, s__, sprintf } from '~/locale';
 import { diffViewerModes } from '~/ide/constants';
 import EditButton from './edit_button.vue';
 import DiffStats from './diff_stats.vue';
-import { scrollToElement } from '~/lib/utils/common_utils';
+import { scrollToElement, contentTop } from '~/lib/utils/common_utils';
 
 export default {
   components: {
@@ -137,9 +137,20 @@ export default {
     isModeChanged() {
       return this.diffFile.viewer.name === diffViewerModes.mode_changed;
     },
+    showExpandDiffToFullFileEnabled() {
+      return gon.features.expandDiffFullFile && !this.diffFile.is_fully_expanded;
+    },
+    expandDiffToFullFileTitle() {
+      if (this.diffFile.isShowingFullFile) {
+        return s__('MRDiff|Show changes only');
+      }
+      return s__('MRDiff|Show full file');
+    },
   },
   mounted() {
     polyfillSticky(this.$refs.header);
+    const fileHeaderHeight = this.$refs.header.clientHeight;
+    stickyMonitor(this.$refs.header, contentTop() - fileHeaderHeight - 1, false);
   },
   methods: {
     ...mapActions('diffs', ['toggleFileDiscussions', 'toggleFullDiff']),
@@ -243,70 +254,70 @@ export default {
       class="file-actions d-none d-sm-block"
     >
       <diff-stats :added-lines="diffFile.added_lines" :removed-lines="diffFile.removed_lines" />
-      <template v-if="diffFile.blob && diffFile.blob.readable_text">
-        <button
-          :disabled="!diffHasDiscussions(diffFile)"
-          :class="{ active: hasExpandedDiscussions }"
-          :title="s__('MergeRequests|Toggle comments for this file')"
-          class="js-btn-vue-toggle-comments btn"
-          type="button"
-          @click="handleToggleDiscussions"
-        >
-          <icon name="comment" />
-        </button>
-
-        <edit-button
-          v-if="!diffFile.deleted_file"
-          :can-current-user-fork="canCurrentUserFork"
-          :edit-path="diffFile.edit_path"
-          :can-modify-blob="diffFile.can_modify_blob"
-          @showForkMessage="showForkMessage"
-        />
-      </template>
+      <div class="btn-group" role="group">
+        <template v-if="diffFile.blob && diffFile.blob.readable_text">
+          <button
+            :disabled="!diffHasDiscussions(diffFile)"
+            :class="{ active: hasExpandedDiscussions }"
+            :title="s__('MergeRequests|Toggle comments for this file')"
+            class="js-btn-vue-toggle-comments btn"
+            type="button"
+            @click="handleToggleDiscussions"
+          >
+            <icon name="comment" />
+          </button>
 
-      <a
-        v-if="diffFile.replaced_view_path"
-        :href="diffFile.replaced_view_path"
-        class="btn view-file js-view-replaced-file"
-        v-html="viewReplacedFileButtonText"
-      >
-      </a>
-      <gl-tooltip :target="() => $refs.viewButton" placement="bottom">
-        <span v-html="viewFileButtonText"></span>
-      </gl-tooltip>
-      <gl-button
-        ref="viewButton"
-        :href="diffFile.view_path"
-        target="blank"
-        class="view-file js-view-file-button"
-      >
-        <icon name="external-link" />
-      </gl-button>
-      <gl-button
-        v-if="!diffFile.is_fully_expanded"
-        class="expand-file js-expand-file"
-        @click="toggleFullDiff(diffFile.file_path)"
-      >
-        <template v-if="diffFile.isShowingFullFile">
-          {{ s__('MRDiff|Show changes only') }}
-        </template>
-        <template v-else>
-          {{ s__('MRDiff|Show full file') }}
+          <edit-button
+            v-if="!diffFile.deleted_file"
+            :can-current-user-fork="canCurrentUserFork"
+            :edit-path="diffFile.edit_path"
+            :can-modify-blob="diffFile.can_modify_blob"
+            @showForkMessage="showForkMessage"
+          />
         </template>
-        <gl-loading-icon v-if="diffFile.isLoadingFullFile" inline />
-      </gl-button>
 
-      <a
-        v-if="diffFile.external_url"
-        v-gl-tooltip.hover
-        :href="diffFile.external_url"
-        :title="`View on ${diffFile.formatted_external_url}`"
-        target="_blank"
-        rel="noopener noreferrer"
-        class="btn btn-file-option js-external-url"
-      >
-        <icon name="external-link" />
-      </a>
+        <a
+          v-if="diffFile.replaced_view_path"
+          :href="diffFile.replaced_view_path"
+          class="btn view-file js-view-replaced-file"
+          v-html="viewReplacedFileButtonText"
+        >
+        </a>
+        <gl-button
+          v-if="!diffFile.is_fully_expanded"
+          ref="expandDiffToFullFileButton"
+          v-gl-tooltip.hover
+          :title="expandDiffToFullFileTitle"
+          class="expand-file js-expand-file"
+          @click="toggleFullDiff(diffFile.file_path)"
+        >
+          <gl-loading-icon v-if="diffFile.isLoadingFullFile" color="dark" inline />
+          <icon v-else-if="diffFile.isShowingFullFile" name="doc-changes" />
+          <icon v-else name="doc-expand" />
+        </gl-button>
+        <gl-button
+          ref="viewButton"
+          v-gl-tooltip.hover
+          :href="diffFile.view_path"
+          target="blank"
+          class="view-file js-view-file-button"
+          title="viewFileButtonText"
+        >
+          <icon name="external-link" />
+        </gl-button>
+
+        <a
+          v-if="diffFile.external_url"
+          v-gl-tooltip.hover
+          :href="diffFile.external_url"
+          :title="`View on ${diffFile.formatted_external_url}`"
+          target="_blank"
+          rel="noopener noreferrer"
+          class="btn btn-file-option js-external-url"
+        >
+          <icon name="external-link" />
+        </a>
+      </div>
     </div>
   </div>
 </template>
diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js
index 6f380fe6ece..5dabe224baa 100644
--- a/app/assets/javascripts/diffs/constants.js
+++ b/app/assets/javascripts/diffs/constants.js
@@ -47,3 +47,6 @@ export const OLD_LINE_KEY = 'old_line';
 export const NEW_LINE_KEY = 'new_line';
 export const TYPE_KEY = 'type';
 export const LEFT_LINE_KEY = 'left';
+
+export const CENTERED_LIMITED_CONTAINER_CLASSES =
+  'container-limited limit-container-width mx-auto px-3';
diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index 59930f8d4a3..2906604da57 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -7,7 +7,7 @@ import axios from './axios_utils';
 import { getLocationHash } from './url_utility';
 import { convertToCamelCase } from './text_utility';
 import { isObject } from './type_utility';
-import BreakpointInstance from '../../breakpoints';
+import breakpointInstance from '../../breakpoints';
 
 export const getPagePath = (index = 0) => {
   const page = $('body').attr('data-page') || '';
@@ -198,11 +198,10 @@ export const contentTop = () => {
   const mrTabsHeight = $('.merge-request-tabs').outerHeight() || 0;
   const headerHeight = $('.navbar-gitlab').outerHeight() || 0;
   const diffFilesChanged = $('.js-diff-files-changed').outerHeight() || 0;
-  const mdScreenOrBigger = ['lg', 'md'].includes(BreakpointInstance.getBreakpointSize());
+  const isDesktop = breakpointInstance.isDesktop();
   const diffFileTitleBar =
-    (mdScreenOrBigger && $('.diff-file .file-title-flex-parent:visible').outerHeight()) || 0;
-  const compareVersionsHeaderHeight =
-    (mdScreenOrBigger && $('.mr-version-controls').outerHeight()) || 0;
+    (isDesktop && $('.diff-file .file-title-flex-parent:visible').outerHeight()) || 0;
+  const compareVersionsHeaderHeight = (isDesktop && $('.mr-version-controls').outerHeight()) || 0;
 
   return (
     perfBar +
diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss
index 8d38310e8e6..53d3645cd63 100644
--- a/app/assets/stylesheets/framework/files.scss
+++ b/app/assets/stylesheets/framework/files.scss
@@ -331,6 +331,10 @@ span.idiff {
     padding: 5px $gl-padding;
     margin: 0;
     border-radius: $border-radius-default $border-radius-default 0 0;
+
+    &.is-stuck {
+      border-radius: 0;
+    }
   }
 
   .file-header-content {
diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss
index 1fedbd8bddb..08e045239e0 100644
--- a/app/assets/stylesheets/pages/diff.scss
+++ b/app/assets/stylesheets/pages/diff.scss
@@ -7,12 +7,15 @@
     cursor: pointer;
 
     @media (min-width: map-get($grid-breakpoints, md)) {
-      $mr-file-header-top: $mr-version-controls-height + $header-height + $mr-tabs-height;
+      // The `-1` below is to prevent two borders from clashing up against eachother -
+      // the bottom of the compare-versions header and the top of the file header
+      $mr-file-header-top: $mr-version-controls-height + $header-height + $mr-tabs-height - 1;
 
       position: -webkit-sticky;
       position: sticky;
       top: $mr-file-header-top;
       z-index: 102;
+      height: $mr-version-controls-height;
 
       &::before {
         content: '';
@@ -54,7 +57,8 @@
       background-color: $gray-normal;
     }
 
-    a {
+    a,
+    button {
       color: $gray-700;
     }
 
diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index 7f8b8ea8100..d241978ce45 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -737,7 +737,6 @@
   background: $gray-light;
   color: $gl-text-color;
   margin-top: -1px;
-  border-top: 1px solid $border-color;
 
   .mr-version-menus-container {
     display: flex;
@@ -760,6 +759,7 @@
 
   .content-block {
     padding: $gl-padding-top $gl-padding;
+    border-bottom: 0;
   }
 
   .comments-disabled-notif {
@@ -784,16 +784,18 @@
     padding-right: 5px;
   }
 
+  // Shortening button height by 1px to make compare-versions
+  // header 56px and fit into our 8px design grid
+  button {
+    height: 34px;
+  }
+
   @include media-breakpoint-up(md) {
     position: -webkit-sticky;
     position: sticky;
     top: $header-height + $mr-tabs-height;
-    width: 100%;
-
-    &.is-fileTreeOpen {
-      margin-left: -16px;
-      width: calc(100% + 32px);
-    }
+    margin-left: -16px;
+    width: calc(100% + 32px);
 
     .mr-version-menus-container {
       flex-wrap: nowrap;
diff --git a/changelogs/unreleased/57364-improve-diff-nav-header.yml b/changelogs/unreleased/57364-improve-diff-nav-header.yml
new file mode 100644
index 00000000000..95d119b949c
--- /dev/null
+++ b/changelogs/unreleased/57364-improve-diff-nav-header.yml
@@ -0,0 +1,5 @@
+---
+title: Make stylistic improvements to diff nav header
+merge_request: 26557
+author:
+type: fixed
diff --git a/spec/javascripts/diffs/components/app_spec.js b/spec/javascripts/diffs/components/app_spec.js
index 8d7c52a2876..3ce69bc3c20 100644
--- a/spec/javascripts/diffs/components/app_spec.js
+++ b/spec/javascripts/diffs/components/app_spec.js
@@ -57,6 +57,24 @@ describe('diffs/components/app', () => {
     wrapper.destroy();
   });
 
+  it('adds container-limiting classes when showFileTree is false with inline diffs', () => {
+    createComponent({}, ({ state }) => {
+      state.diffs.showTreeList = false;
+      state.diffs.isParallelView = false;
+    });
+
+    expect(wrapper.contains('.container-limited.limit-container-width')).toBe(true);
+  });
+
+  it('does not add container-limiting classes when showFileTree is false with inline diffs', () => {
+    createComponent({}, ({ state }) => {
+      state.diffs.showTreeList = true;
+      state.diffs.isParallelView = false;
+    });
+
+    expect(wrapper.contains('.container-limited.limit-container-width')).toBe(false);
+  });
+
   it('displays loading icon on loading', () => {
     createComponent({}, ({ state }) => {
       state.diffs.isLoading = true;
diff --git a/spec/javascripts/diffs/components/compare_versions_spec.js b/spec/javascripts/diffs/components/compare_versions_spec.js
index e886f962d2f..77f8352047c 100644
--- a/spec/javascripts/diffs/components/compare_versions_spec.js
+++ b/spec/javascripts/diffs/components/compare_versions_spec.js
@@ -66,6 +66,26 @@ describe('CompareVersions', () => {
       expect(inlineBtn.innerHTML).toContain('Inline');
       expect(parallelBtn.innerHTML).toContain('Side-by-side');
     });
+
+    it('adds container-limiting classes when showFileTree is false with inline diffs', () => {
+      vm.isLimitedContainer = true;
+
+      vm.$nextTick(() => {
+        const limitedContainer = vm.$el.querySelector('.container-limited.limit-container-width');
+
+        expect(limitedContainer).not.toBeNull();
+      });
+    });
+
+    it('does not add container-limiting classes when showFileTree is false with inline diffs', () => {
+      vm.isLimitedContainer = false;
+
+      vm.$nextTick(() => {
+        const limitedContainer = vm.$el.querySelector('.container-limited.limit-container-width');
+
+        expect(limitedContainer).toBeNull();
+      });
+    });
   });
 
   describe('setInlineDiffViewType', () => {
diff --git a/spec/javascripts/diffs/components/diff_file_header_spec.js b/spec/javascripts/diffs/components/diff_file_header_spec.js
index 6614069f44d..e1170c9762e 100644
--- a/spec/javascripts/diffs/components/diff_file_header_spec.js
+++ b/spec/javascripts/diffs/components/diff_file_header_spec.js
@@ -672,7 +672,7 @@ describe('diff_file_header', () => {
 
       vm = mountComponentWithStore(Component, { props, store });
 
-      expect(vm.$el.querySelector('.js-expand-file').textContent).toContain('Show changes only');
+      expect(vm.$el.querySelector('.ic-doc-changes')).not.toBeNull();
     });
 
     it('shows expand text', () => {
@@ -680,7 +680,7 @@ describe('diff_file_header', () => {
 
       vm = mountComponentWithStore(Component, { props, store });
 
-      expect(vm.$el.querySelector('.js-expand-file').textContent).toContain('Show full file');
+      expect(vm.$el.querySelector('.ic-doc-expand')).not.toBeNull();
     });
 
     it('renders loading icon', () => {
diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js
index 2084c36e484..da012e1d5f7 100644
--- a/spec/javascripts/lib/utils/common_utils_spec.js
+++ b/spec/javascripts/lib/utils/common_utils_spec.js
@@ -2,7 +2,7 @@ import axios from '~/lib/utils/axios_utils';
 import * as commonUtils from '~/lib/utils/common_utils';
 import MockAdapter from 'axios-mock-adapter';
 import { faviconDataUrl, overlayDataUrl, faviconWithOverlayDataUrl } from './mock_data';
-import BreakpointInstance from '~/breakpoints';
+import breakpointInstance from '~/breakpoints';
 
 const PIXEL_TOLERANCE = 0.2;
 
@@ -383,7 +383,7 @@ describe('common_utils', () => {
 
   describe('contentTop', () => {
     it('does not add height for fileTitle or compareVersionsHeader if screen is too small', () => {
-      spyOn(BreakpointInstance, 'getBreakpointSize').and.returnValue('sm');
+      spyOn(breakpointInstance, 'isDesktop').and.returnValue(false);
 
       setFixtures(`
         <div class="diff-file file-title-flex-parent">
@@ -398,7 +398,7 @@ describe('common_utils', () => {
     });
 
     it('adds height for fileTitle and compareVersionsHeader screen is large enough', () => {
-      spyOn(BreakpointInstance, 'getBreakpointSize').and.returnValue('lg');
+      spyOn(breakpointInstance, 'isDesktop').and.returnValue(true);
 
       setFixtures(`
         <div class="diff-file file-title-flex-parent">
-- 
cgit v1.2.1