From fb6eddee1bdeda4758f706bb0773d15f35859d2c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 6 Jan 2025 18:10:25 -0700 Subject: [PATCH 1/3] Toggle Rapid Diffs diff files --- app/assets/javascripts/rapid_diffs/adapters.js | 7 +++++-- .../javascripts/rapid_diffs/toggle_file/adapter.js | 9 +++++++++ .../components/rapid_diffs/diff_file_component.scss | 12 ++++++++++++ .../rapid_diffs/diff_file_component.html.haml | 6 ++++-- .../rapid_diffs/diff_file_header_component.html.haml | 3 +++ 5 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 app/assets/javascripts/rapid_diffs/toggle_file/adapter.js diff --git a/app/assets/javascripts/rapid_diffs/adapters.js b/app/assets/javascripts/rapid_diffs/adapters.js index 3ba1e747e3ba60..fb9c404263e149 100644 --- a/app/assets/javascripts/rapid_diffs/adapters.js +++ b/app/assets/javascripts/rapid_diffs/adapters.js @@ -1,11 +1,14 @@ import { ExpandLinesAdapter } from '~/rapid_diffs/expand_lines/adapter'; +import { ToggleFileAdapter } from '~/rapid_diffs/toggle_file/adapter'; const RAPID_DIFFS_VIEWERS = { text_inline: 'text_inline', text_parallel: 'text_parallel', }; +const COMMON_ADAPTERS = [ExpandLinesAdapter, ToggleFileAdapter]; + export const VIEWER_ADAPTERS = { - [RAPID_DIFFS_VIEWERS.text_inline]: [ExpandLinesAdapter], - [RAPID_DIFFS_VIEWERS.text_parallel]: [ExpandLinesAdapter], + [RAPID_DIFFS_VIEWERS.text_inline]: COMMON_ADAPTERS, + [RAPID_DIFFS_VIEWERS.text_parallel]: COMMON_ADAPTERS, }; diff --git a/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js b/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js new file mode 100644 index 00000000000000..31017e06d1fbf1 --- /dev/null +++ b/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js @@ -0,0 +1,9 @@ +export const ToggleFileAdapter = { + clicks: { + toggleFile() { + const fileBody = this.diffElement.querySelector('[data-file-body]'); + + fileBody.hidden = !fileBody.hidden; + }, + }, +}; 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 6069cd92e1d360..bff4a9184523fa 100644 --- a/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss +++ b/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss @@ -2,6 +2,11 @@ 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 { @@ -36,3 +41,10 @@ border-radius: inherit; } } + +[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/components/rapid_diffs/diff_file_component.html.haml b/app/components/rapid_diffs/diff_file_component.html.haml index a4cfca8fdb5278..948278b97c2a4a 100644 --- a/app/components/rapid_diffs/diff_file_component.html.haml +++ b/app/components/rapid_diffs/diff_file_component.html.haml @@ -3,6 +3,8 @@ %diff-file{ id: id, data: server_data } .rd-diff-file = render RapidDiffs::DiffFileHeaderComponent.new(diff_file: @diff_file) - .rd-diff-file-body - = render viewer_component.new(diff_file: @diff_file) + -# extra wrapper needed so content-visibility: hidden doesn't require removing border or other styles + %div{ data: { file_body: '' } } + .rd-diff-file-body + = render viewer_component.new(diff_file: @diff_file) %diff-file-mounted 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 155aecfffcaba5..3d328292654c1d 100644 --- a/app/components/rapid_diffs/diff_file_header_component.html.haml +++ b/app/components/rapid_diffs/diff_file_header_component.html.haml @@ -13,6 +13,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-title - if @diff_file.submodule? %span -- GitLab From 03bb39ae11513fea20d5fc8bca29aa66da39375d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 7 Jan 2025 10:30:36 -0700 Subject: [PATCH 2/3] Properly manage button focus when the DOM visibility changes --- .../javascripts/rapid_diffs/toggle_file/adapter.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js b/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js index 31017e06d1fbf1..13185213b780ba 100644 --- a/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js +++ b/app/assets/javascripts/rapid_diffs/toggle_file/adapter.js @@ -1,9 +1,21 @@ +function oppositeToggleButton(clicked) { + const isOpened = clicked.dataset.opened; + const parent = clicked.parentElement; + + return isOpened === '' + ? parent.querySelector('button[data-closed]') + : parent.querySelector('button[data-opened]'); +} + export const ToggleFileAdapter = { clicks: { - toggleFile() { + toggleFile(event) { const fileBody = this.diffElement.querySelector('[data-file-body]'); + const button = event.target.closest('button'); + const oppositeButton = oppositeToggleButton(button); fileBody.hidden = !fileBody.hidden; + oppositeButton.focus(); }, }, }; -- GitLab From f577520ba7a2066f6580cd97d84cb6ec7c2b069d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 13 Jan 2025 15:28:16 -0700 Subject: [PATCH 3/3] Add a test for our diff file toggle behavior --- .../rapid_diffs/toggle_file/adapter_spec.js | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 spec/frontend/rapid_diffs/toggle_file/adapter_spec.js diff --git a/spec/frontend/rapid_diffs/toggle_file/adapter_spec.js b/spec/frontend/rapid_diffs/toggle_file/adapter_spec.js new file mode 100644 index 00000000000000..45c6085777f29d --- /dev/null +++ b/spec/frontend/rapid_diffs/toggle_file/adapter_spec.js @@ -0,0 +1,66 @@ +import { DiffFile } from '~/rapid_diffs/diff_file'; +import { ToggleFileAdapter } from '~/rapid_diffs/toggle_file/adapter'; + +describe('Diff File Toggle Behavior', () => { + // In our version of Jest/JSDOM we cannot use + // + // - CSS "&" nesting (baseline 2023) + // - Element.checkVisibility (baseline 2024) + // - :has (baseline 2023) + // + // so this cannot test CSS (which is a majority of our behavior), and must assume that + // browser CSS is working as documented when we tweak HTML attributes + const html = ` + +
+
+
< + + +
+
+
+ + + `; + + function get(element) { + const elements = { + file: () => document.querySelector('diff-file'), + hide: () => get('file').querySelector('button[data-opened]'), + show: () => get('file').querySelector('button[data-closed]'), + body: () => get('file').querySelector('[data-file-body]'), + }; + + return elements[element]?.(); + } + + function assignAdapter(customAdapter) { + get('file').adapterConfig = { any: [customAdapter] }; + } + + beforeAll(() => { + customElements.define('diff-file', DiffFile); + }); + + beforeEach(() => { + document.body.innerHTML = html; + assignAdapter(ToggleFileAdapter); + get('file').mount(); + }); + + it('starts with the file body visible', () => { + expect(get('body').hidden).toEqual(false); + }); + + it('marks the body hidden and focuses the other button when the hide button is clicked', () => { + const show = get('show'); + const hide = get('hide'); + const body = get('body'); + + hide.click(); + + expect(body.hidden).toEqual(true); + expect(document.activeElement).toEqual(show); + }); +}); -- GitLab