diff --git a/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js b/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js index 871ce8ea100d43ebffda3df8519a2fbe5fd7576b..3060610ae07ac1546ab740306de83116606f3f6d 100644 --- a/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js +++ b/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]() { diff --git a/app/assets/stylesheets/components/rapid_diffs/_constants.scss b/app/assets/stylesheets/components/rapid_diffs/_constants.scss index f2fd732304b4ba567da7d4251b1385d7bee5b41f..7c6912d283f224b159cf99861902831eebedb6a1 100644 --- a/app/assets/stylesheets/components/rapid_diffs/_constants.scss +++ b/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 diff --git a/app/assets/stylesheets/components/rapid_diffs/app.scss b/app/assets/stylesheets/components/rapid_diffs/app.scss index 485b17a0af800207c7df25780d677caa60ddb853..0f4c0f837819a18bd8ce1a30e38af68c76ccba38 100644 --- a/app/assets/stylesheets/components/rapid_diffs/app.scss +++ b/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 { diff --git a/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss b/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss index 7452c1d7c2c6a2913fdd567383ecf9a1184b6ec2..8856f8899297aeb6fbd961be0ad12d6e86fab0df 100644 --- a/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss +++ b/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; + } } + diff --git a/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss b/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss index 5264f3343d000511d12eca45dc2b2f76f468b947..65707eeb4a9b9cf0010ea1553deb672b6e760e3a 100644 --- a/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss +++ b/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss @@ -1,2 +1,6 @@ @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}); +} diff --git a/app/components/rapid_diffs/diff_file_header_component.html.haml b/app/components/rapid_diffs/diff_file_header_component.html.haml index 02b5ef07ab278448131ee29a8434a170f210d221..e0411e679fc409610b8378bb030acbdd6d6e23c7 100644 --- a/app/components/rapid_diffs/diff_file_header_component.html.haml +++ b/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>= "−".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>= "−".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') } }) diff --git a/spec/frontend/rapid_diffs/toggle_file/adapter_spec.js b/spec/frontend/rapid_diffs/toggle_file/adapter_spec.js index 3db30bfe22ab2d22298ffbae3b1297a6456879e9..a0d74449a1d64e5a4553501b3dfacf63f06a46f3 100644 --- a/spec/frontend/rapid_diffs/toggle_file/adapter_spec.js +++ b/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'); }); });