Merge branch 'slashmanov/rapid-diffs-sticky-header' into 'master'

Update Rapid Diffs file header

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/184007



Merged-by: Phil Hughes <me@iamphill.com>
Approved-by: Thomas Randolph <trandolph@gitlab.com>
Approved-by: Phil Hughes <me@iamphill.com>
Co-authored-by: Stanislav Lashmanov <slashmanov@gitlab.com>

app/assets/javascripts/rapid_diffs/toggle_file/adapter.js

@@ -9,33 +9,30 @@ function getOppositeToggleButton(clicked) {
     : parent.querySelector('button[data-opened]');
 }
 
-function getElements(diffElement) {
-  const fileBody = diffElement.querySelector('[data-file-body]');
-  const button = diffElement.querySelector('[data-click="toggleFile"]');
-  const oppositeToggleButton = getOppositeToggleButton(button);
-  return { fileBody, oppositeToggleButton };
-}
-
-function collapse(fileBody = this.diffElement.querySelector('[data-file-body]')) {
+function collapse(root = this.diffElement) {
   // eslint-disable-next-line no-param-reassign
-  fileBody.hidden = true;
+  root.dataset.collapsed = true;
+  // eslint-disable-next-line no-param-reassign
+  root.querySelector('[data-file-body]').hidden = true;
 }
 
-function expand(fileBody = this.diffElement.querySelector('[data-file-body]')) {
+function expand(root = this.diffElement) {
+  // eslint-disable-next-line no-param-reassign
+  delete root.dataset.collapsed;
   // eslint-disable-next-line no-param-reassign
-  fileBody.hidden = false;
+  root.querySelector('[data-file-body]').hidden = false;
 }
 
 export const ToggleFileAdapter = {
   clicks: {
-    toggleFile() {
-      const { fileBody, oppositeToggleButton } = getElements(this.diffElement);
-      if (fileBody.hidden) {
-        expand.call(this, fileBody);
+    toggleFile(event, button) {
+      const collapsed = this.diffElement.dataset.collapsed === 'true';
+      if (collapsed) {
+        expand.call(this);
       } else {
-        collapse.call(this, fileBody);
+        collapse.call(this);
       }
-      oppositeToggleButton.focus();
+      getOppositeToggleButton(button).focus();
     },
   },
   [EXPAND_FILE]() {

app/assets/stylesheets/components/rapid_diffs/_constants.scss

@@ -6,4 +6,4 @@ $code-row-height: calc($code-row-height-target / $base-font-size * 1rem);
 $code-font-size: calc($code-font-size-target / $base-font-size * 1rem);
 $code-line-height: calc($code-row-height-target / $code-font-size-target);
 
-$diff-file-sticky-offset: 11px
+$diff-file-sticky-top-margin: 11px

app/assets/stylesheets/components/rapid_diffs/app.scss

@@ -8,7 +8,9 @@
   min-height: 3rem;
   // do not let dropdowns cover sticky header
   position: relative;
-  z-index: 1;
+  // this should always be higher than z-index inside diff files or file browser
+  // otherwise dropdowns will not cover diff file or file tree content
+  z-index: 10;
 }
 
 .rd-app-settings {

app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss

@@ -2,22 +2,20 @@
 @import 'framework/variables';
 
 .rd-diff-file-component {
-  // TODO: this must be defined using CSS Custom Properties to work across all pages
-  scroll-margin-top: calc(#{$calc-application-header-height} + #{$mr-sticky-header-height} + #{constants.$diff-file-sticky-offset});
+  --rd-diff-file-sticky-top-offset: calc(var(--rd-diff-file-sticky-top-padding, 0) + #{constants.$diff-file-sticky-top-margin});
+  scroll-margin-top: var(--rd-diff-file-sticky-top-offset);
 }
 
 .rd-diff-file {
   padding-bottom: $gl-padding;
 
   --rd-diff-file-border-radius: #{calc($gl-border-radius-base - 1px)};
-
-  &:has([data-file-body][hidden]) .rd-diff-file-toggle [data-opened],
-  &:not(:has([data-file-body][hidden])) .rd-diff-file-toggle [data-closed] {
-    display: none;
-  }
 }
 
 .rd-diff-file-header {
+  position: sticky;
+  // 1px offset to hide top border
+  top: calc(var(--rd-diff-file-sticky-top-padding, 1px) - 1px);
   display: flex;
   background-color: var(--gl-background-color-subtle);
   border: 1px solid var(--gl-border-color-default);
@@ -25,9 +23,53 @@
   min-height: px-to-rem($file-header-height);
   border-radius: var(--rd-diff-file-border-radius) var(--rd-diff-file-border-radius) 0 0;
   word-break: break-word;
+  z-index: 1;
+}
+
+.rd-diff-file[data-collapsed] .rd-diff-file-header {
+  border-radius: var(--rd-diff-file-border-radius);
+}
+
+.rd-diff-file-title {
+  position: relative;
+  display: flex;
+  align-items: center;
+  // extra spacing to avoid accidental file collapse clicks
+  padding: $gl-spacing-scale-3;
+  margin: -$gl-spacing-scale-3;
+  gap: $gl-padding-4;
+  z-index: 1;
+}
+
+.rd-diff-file-toggle {
+  margin-right: $gl-spacing-scale-2;
+}
+
+.rd-diff-file[data-collapsed] .rd-diff-file-toggle-button[data-opened],
+.rd-diff-file:not([data-collapsed]) .rd-diff-file-toggle-button[data-closed] {
+  display: none;
+}
+
+.rd-diff-file-toggle-button::before {
+  content: '';
+  position: absolute;
+  inset: 0;
+}
+
+.rd-diff-file-info {
+  position: relative;
+  display: flex;
+  // extra spacing to avoid accidental file collapse clicks
+  padding: $gl-spacing-scale-3;
+  margin: -$gl-spacing-scale-3;
+  margin-left: auto;
+  gap: $gl-padding;
+  z-index: 1;
 }
 
 .rd-diff-file-stats {
+  display: flex;
+  align-items: center;
   margin-left: auto;
 }
 
@@ -37,7 +79,7 @@
 
 .rd-lines-removed {
   @apply gl-text-danger;
-  margin-left: $gl-padding-4;
+  margin-left: $gl-padding-8;
 }
 
 .rd-diff-file-body {
@@ -50,9 +92,12 @@
   }
 }
 
-[data-file-body][hidden] {
-  display: block !important;
-  // https://web.dev/articles/content-visibility#hide_content_with_content-visibility_hidden
-  // content-visibility: hidden preserves element's rendering state which improves performance for larger diffs
-  content-visibility: hidden;
+@supports (content-visibility: hidden) {
+  [data-file-body][hidden] {
+    display: block !important;
+    // https://web.dev/articles/content-visibility#hide_content_with_content-visibility_hidden
+    // content-visibility: hidden preserves element's rendering state which improves performance for larger diffs
+    content-visibility: hidden;
+  }
 }
+

app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss

 @import 'mixins_and_variables_and_functions';
 @import 'components/rapid_diffs';
+
+.rd-app {
+  --rd-diff-file-sticky-top-padding: calc(#{$calc-application-header-height} + #{$mr-sticky-header-height});
+}

app/components/rapid_diffs/diff_file_header_component.html.haml

@@ -12,9 +12,9 @@
 -#  * submodule compare
 
 .rd-diff-file-header{ data: { testid: 'rd-diff-file-header' } }
-  .rd-diff-file-toggle.gl-mr-2<
-    = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, icon: 'chevron-down', button_options: { data: { opened: '', click: 'toggleFile' }, aria: { label: _('Hide file contents') } })
-    = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, icon: 'chevron-right', button_options: { data: { closed: '', click: 'toggleFile' }, aria: { label: _('Show file contents') } })
+  .rd-diff-file-toggle<
+    = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, icon: 'chevron-down', button_options: { class: 'rd-diff-file-toggle-button', data: { opened: '', click: 'toggleFile' }, aria: { label: _('Hide file contents') } })
+    = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, icon: 'chevron-right', button_options: { class: 'rd-diff-file-toggle-button', data: { closed: '', click: 'toggleFile' }, aria: { label: _('Show file contents') } })
   .rd-diff-file-title
     - if @diff_file.submodule?
       %span{ data: { testid: 'rd-diff-file-header-submodule' } }
@@ -41,13 +41,14 @@
         %small #{@diff_file.a_mode} → #{@diff_file.b_mode}
       - if @diff_file.stored_externally? && @diff_file.external_storage == :lfs
         = helpers.gl_badge_tag(_('LFS'), variant: :neutral)
-  .rd-diff-file-stats
-    %span.rd-lines-added
-      %span>= "+"
-      %span{ "data-testid" => "js-file-addition-line" }= @diff_file.added_lines
-    %span.rd-lines-removed
-      %span>= "&#x2212;".html_safe
-      %span{ "data-testid" => "js-file-deletion-line" }= @diff_file.removed_lines
-  .rd-diff-file-options-menu.gl-ml-2
-    .js-options-menu
-      = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, icon: 'ellipsis_v', button_options: { class: 'js-options-button', data: { click: 'toggleOptionsMenu' }, aria: { label: _('Options') } })
+  .rd-diff-file-info
+    .rd-diff-file-stats
+      %span.rd-lines-added
+        %span>= "+"
+        %span{ "data-testid" => "js-file-addition-line" }= @diff_file.added_lines
+      %span.rd-lines-removed
+        %span>= "&#x2212;".html_safe
+        %span{ "data-testid" => "js-file-deletion-line" }= @diff_file.removed_lines
+    .rd-diff-file-options-menu
+      .js-options-menu
+        = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, icon: 'ellipsis_v', button_options: { class: 'js-options-button', data: { click: 'toggleOptionsMenu' }, aria: { label: _('Options') } })

spec/frontend/rapid_diffs/toggle_file/adapter_spec.js

@@ -68,10 +68,12 @@ describe('Diff File Toggle Behavior', () => {
   it('collapses file', () => {
     get('file').trigger(COLLAPSE_FILE);
     expect(get('body').hidden).toEqual(true);
+    expect(get('file').diffElement.dataset.collapsed).toEqual('true');
   });
 
   it('expands file', () => {
     get('file').trigger(EXPAND_FILE);
     expect(get('body').hidden).toEqual(false);
+    expect(get('file').diffElement.dataset.collapsed).not.toEqual('true');
   });
 });