diff --git a/app/assets/javascripts/merge_request.js b/app/assets/javascripts/merge_request.js index 03bfc4a9602e8bd8f1a3e65ade5a7b88a242da67..db4788b3d37a048855bc55488b25a0cb074111d2 100644 --- a/app/assets/javascripts/merge_request.js +++ b/app/assets/javascripts/merge_request.js @@ -12,6 +12,9 @@ import { addDelimiter } from './lib/utils/text_utility'; import { getParameterValues, setUrlParams } from './lib/utils/url_utility'; import TaskList from './task_list'; +const FONT_SIZE = parseFloat(getComputedStyle(document.documentElement).fontSize); +const VH = document.documentElement.clientHeight; + function MergeRequest(opts) { // Initialize MergeRequest behavior // @@ -25,6 +28,11 @@ function MergeRequest(opts) { this.initTabs(); this.initMRBtnListeners(); + this.truncation.prepare(); + if (window.gon.features.mergeRequestCollapsedDescription) { + this.truncation.truncateDescription(); + } + if ($('.description.js-task-list-container').length) { this.taskList = new TaskList({ dataType: TYPE_MERGE_REQUEST, @@ -137,6 +145,45 @@ MergeRequest.prototype.submitNoteForm = function (form, $button) { } }; +MergeRequest.prototype.truncation = { + /* + max also implemented in assets/stylesheets/page_bundles/merge_request.scss. + Ensure they are synced to avoid buggy behavior + */ + max: Math.min( + Math.max(16 * FONT_SIZE /* 16rem */, 0.4 * VH /* 40vh */), + 32 * FONT_SIZE /* 32rem */, + ), + description: null, + markdown: null, + button: null, + loader: null, + binding: null, + listener() { + if (this.md && this.button) { + this.md.classList.remove('truncated'); + this.button.removeEventListener('click', this.binding); + } + }, + truncateDescription() { + this.binding = this.listener.bind(this); + + if (this.md.getBoundingClientRect().height > this.max) { + this.md.classList.add('truncated'); + this.button.addEventListener('click', this.binding); + } + }, + prepare() { + this.description = document.querySelector('.description'); + this.md = this.description.querySelector('.md'); + this.button = this.description.querySelector('.untruncate-button button.gl-button'); + this.loader = document.querySelector('.description + .gl-mt-3'); + + this.description.classList.remove('gl-hidden'); + this.loader?.remove(); + }, +}; + MergeRequest.decreaseCounter = function (by = 1) { const $el = $('.js-merge-counter'); const count = Math.max(parseInt($el.first().text().replace(/[^\d]/, ''), 10) - by, 0); diff --git a/app/assets/stylesheets/page_bundles/merge_request.scss b/app/assets/stylesheets/page_bundles/merge_request.scss index 4219bd25f916728debbe81ba643f4cb87ebf6b4e..0da0f1111b18554f47cd1f1e07480c422981322a 100644 --- a/app/assets/stylesheets/page_bundles/merge_request.scss +++ b/app/assets/stylesheets/page_bundles/merge_request.scss @@ -336,3 +336,58 @@ $comparison-empty-state-height: 62px; width: 100%; } } + +.description{ + .description-untruncate{ + bottom: 0; + display: none; + pointer-events: none; + position: absolute; + + &::before { + content: ''; + display: block; + height: 3rem; + width: 100%; + background: linear-gradient(180deg, rgba(255, 255, 255, 0.00) 0%, $white 100%); + + .gl-dark & { + background: linear-gradient(180deg, rgba(31, 30, 36, 0.00) 0%, $gray-950 100%); + } + } + } + + .md.truncated{ + /* + max-height/clamp also implemented in javascripts/merge_request.js. + Ensure they are synced to avoid buggy behavior + */ + max-height: clamp(16rem, 40vh, 32rem); + overflow: hidden; + + + .description-untruncate{ + display: block; + + .untruncate-button { + pointer-events: auto; + background-color: $white; + + .gl-dark & { + background-color: $gray-950; + } + + &::before, &::after { + content: ''; + align-self: center; + height: 1px; + flex: 1; + border-top: 1px solid $gray-50; + + .gl-dark & { + border-top: 1px solid $gray-900; + } + } + } + } + } +} diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 14d8b2c16740c192a3293326900b868fa54aa88e..f9ae9ac373ce2dba67b908c241269001978a1d6a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -45,6 +45,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:notifications_todos_buttons, current_user) push_frontend_feature_flag(:pinned_file, project) push_frontend_feature_flag(:reviewer_assign_drawer, current_user) + push_frontend_feature_flag(:merge_request_collapsed_description, project) end around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :diffs, :rapid_diffs, :discussions] diff --git a/app/views/projects/merge_requests/_description.html.haml b/app/views/projects/merge_requests/_description.html.haml index 89eed0789e82503cf7ea21872368ef4f04c362d1..971481e3df17461efd03a42a1e09450f50eb06e3 100644 --- a/app/views/projects/merge_requests/_description.html.haml +++ b/app/views/projects/merge_requests/_description.html.haml @@ -1,8 +1,19 @@ +- description_classes = ['gl-relative', 'gl-clearfix', '!gl-mt-4', can?(current_user, :update_merge_request, @merge_request) ? 'js-task-list-container' : ''] +- if Feature.enabled?(:merge_request_collapsed_description, @merge_request) + - description_classes.append( 'gl-hidden' ) + %div - if @merge_request.description.present? - .description{ class: ['gl-mt-4!', can?(current_user, :update_merge_request, @merge_request) ? 'js-task-list-container' : ''], data: { testid: 'description-content' } } + .description{ class: description_classes, data: { testid: 'description-content' } } .md = markdown_field(@merge_request, :description) + - if Feature.enabled?(:merge_request_collapsed_description, @merge_request) + .description-untruncate.gl-block.gl-w-full + .untruncate-button.gl-w-full.gl-flex + = render Pajamas::ButtonComponent.new(variant: :confirm, category: :tertiary, button_options: { class: 'gl-mx-4' }) do + = _('Read more') %textarea.hidden.js-task-list-field{ data: { value: @merge_request.description } } - + - if Feature.enabled?(:merge_request_collapsed_description, @merge_request) + .gl-mt-3 + = gl_loading_icon(size: 'lg') = edited_time_ago_with_tooltip(@merge_request, placement: 'bottom') diff --git a/config/feature_flags/gitlab_com_derisk/merge_request_collapsed_description.yml b/config/feature_flags/gitlab_com_derisk/merge_request_collapsed_description.yml new file mode 100644 index 0000000000000000000000000000000000000000..f794877d2b49eb31ad22bf143b8717375ff923b8 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/merge_request_collapsed_description.yml @@ -0,0 +1,9 @@ +--- +name: merge_request_collapsed_description +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/430730 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/156100 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/467136 +milestone: '17.1' +group: group::code review +type: gitlab_com_derisk +default_enabled: false diff --git a/spec/frontend/merge_request_spec.js b/spec/frontend/merge_request_spec.js index a119ca8272e74bf76983b00b1895792b7ef9664b..82173fe5407864986458e9e8889dab15fff31ee9 100644 --- a/spec/frontend/merge_request_spec.js +++ b/spec/frontend/merge_request_spec.js @@ -15,6 +15,10 @@ describe('MergeRequest', () => { const test = {}; describe('task lists', () => { let mock; + let truncateSpy; + let originalTruncate; + let prepareSpy; + let originalPrepare; beforeEach(() => { setHTMLFixture(htmlMergeRequestWithTaskList); @@ -26,12 +30,26 @@ describe('MergeRequest', () => { .onPatch(`${TEST_HOST}/frontend-fixtures/merge-requests-project/-/merge_requests/1.json`) .reply(HTTP_STATUS_OK, {}); + window.gon.features = { mergeRequestCollapsedDescription: true }; + + // Spy on the truncation + originalTruncate = MergeRequest.prototype.truncation.truncateDescription; + truncateSpy = jest.fn(originalTruncate); + MergeRequest.prototype.truncation.truncateDescription = truncateSpy; + + // Spy on the truncation preparer + originalPrepare = MergeRequest.prototype.truncation.prepare; + prepareSpy = jest.fn(originalPrepare); + MergeRequest.prototype.truncation.prepare = prepareSpy; + test.merge = new MergeRequest(); return test.merge; }); afterEach(() => { mock.restore(); + MergeRequest.prototype.truncation.truncateDescription = originalTruncate; + MergeRequest.prototype.truncation.prepare = originalPrepare; resetHTMLFixture(); }); @@ -108,5 +126,22 @@ describe('MergeRequest', () => { ); }); }); + + describe('truncation', () => { + /* + Documents are not actually rendered/painted in tests, so we can't test the logic with the actual height limits (everything is 0) + + That said, it's _almost_ all browser APIs, so there's not much custom logic at work. + */ + it('collapses descriptions when the feature flag is enabled', () => { + expect(truncateSpy).toHaveBeenCalledTimes(1); + }); + + it('removes the statically-rendered classes/elements that hide the description', () => { + expect(prepareSpy).toHaveBeenCalledTimes(1); + expect(document.querySelector('.description').classList.contains('gl-hidden')).toBe(false); + expect(document.querySelector('.description + .gl-mt-3')).toBe(null); + }); + }); }); });