From f3487985aad34d10b4a1df36b05c9cca14dcfcae Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 3 Jan 2023 14:48:58 -0700 Subject: [PATCH 01/12] Switch draft state toggle to use a checkbox Changelog: changed --- .../javascripts/issuable/issuable_form.js | 58 ++++++++----------- .../shared/issuable/form/_title.html.haml | 21 ++----- 2 files changed, 31 insertions(+), 48 deletions(-) diff --git a/app/assets/javascripts/issuable/issuable_form.js b/app/assets/javascripts/issuable/issuable_form.js index 99a3f76ca764d5..c885d0367fa0ad 100644 --- a/app/assets/javascripts/issuable/issuable_form.js +++ b/app/assets/javascripts/issuable/issuable_form.js @@ -60,8 +60,6 @@ export default class IssuableForm { return; } this.form = form; - this.toggleWip = this.toggleWip.bind(this); - this.renderWipExplanation = this.renderWipExplanation.bind(this); this.resetAutosave = this.resetAutosave.bind(this); this.handleSubmit = this.handleSubmit.bind(this); // prettier-ignore @@ -86,6 +84,7 @@ export default class IssuableForm { this.fallbackKey = getFallbackKey(); this.titleField = this.form.find('input[name*="[title]"]'); this.descriptionField = this.form.find('textarea[name*="[description]"]'); + this.draftCheck = document.querySelector('input.js-toggle-draft'); if (!(this.titleField.length && this.descriptionField.length)) { return; } @@ -93,8 +92,7 @@ export default class IssuableForm { this.autosaves = this.initAutosave(); this.form.on('submit', this.handleSubmit); this.form.on('click', '.btn-cancel, .js-reset-autosave', this.resetAutosave); - this.form.find('.js-unwrap-on-load').unwrap(); - this.initWip(); + this.initDraft(); const $issuableDueDate = $('#issuable-due-date'); @@ -160,48 +158,42 @@ export default class IssuableForm { }); } - initWip() { - this.$wipExplanation = this.form.find('.js-wip-explanation'); - this.$noWipExplanation = this.form.find('.js-no-wip-explanation'); - if (!(this.$wipExplanation.length && this.$noWipExplanation.length)) { - return undefined; - } - this.form.on('click', '.js-toggle-wip', this.toggleWip); - this.titleField.on('keyup blur', this.renderWipExplanation); - return this.renderWipExplanation(); + initDraft() { + this.form.on('click', '.js-toggle-draft', () => this.writeDraftStatus()); + this.titleField.on('keyup blur', () => this.readDraftStatus()); + + this.readDraftStatus(); } - workInProgress() { + isMarkedDraft() { return this.draftRegex.test(this.titleField.val()); } - - renderWipExplanation() { - if (this.workInProgress()) { - // These strings are not "translatable" (the code is hard-coded to look for them) - this.$wipExplanation.find('code')[0].textContent = - 'Draft'; /* eslint-disable-line @gitlab/require-i18n-strings */ - this.$wipExplanation.show(); - return this.$noWipExplanation.hide(); + readDraftStatus() { + if (this.isMarkedDraft()) { + this.checkDraft(); + } else { + this.uncheckDraft(); } - this.$wipExplanation.hide(); - return this.$noWipExplanation.show(); } - - toggleWip(event) { - event.preventDefault(); - if (this.workInProgress()) { - this.removeWip(); + writeDraftStatus() { + if (this.draftCheck.checked) { + this.addDraft(); } else { - this.addWip(); + this.removeDraft(); } - return this.renderWipExplanation(); } - removeWip() { + removeDraft() { return this.titleField.val(this.titleField.val().replace(this.draftRegex, '')); } + uncheckDraft() { + this.draftCheck.checked = false; + } - addWip() { + addDraft() { this.titleField.val(`Draft: ${this.titleField.val()}`); } + checkDraft() { + this.draftCheck.checked = true; + } } diff --git a/app/views/shared/issuable/form/_title.html.haml b/app/views/shared/issuable/form/_title.html.haml index 0f6ef33d5324ee..4d31baee25b0e4 100644 --- a/app/views/shared/issuable/form/_title.html.haml +++ b/app/views/shared/issuable/form/_title.html.haml @@ -1,27 +1,18 @@ - issuable = local_assigns.fetch(:issuable) -- has_wip_commits = local_assigns.fetch(:has_wip_commits) - form = local_assigns.fetch(:form) - no_issuable_templates = issuable_templates(ref_project, issuable.to_ability_name).empty? -- toggle_wip_link_start = '' -- toggle_wip_link_end = '' -- add_wip_text = (_('%{link_start}Start the title with %{draft_snippet}%{link_end} to prevent a merge request draft from merging before it\'s ready.') % { link_start: toggle_wip_link_start, link_end: toggle_wip_link_end, draft_snippet: 'Draft:'.html_safe }).html_safe -- remove_wip_text = (_('%{link_start}Remove the %{draft_snippet} prefix%{link_end} from the title to allow this merge request to be merged when it\'s ready.') % { link_start: toggle_wip_link_start, link_end: toggle_wip_link_end, draft_snippet: 'Draft'.html_safe }).html_safe %div{ data: { testid: 'issue-title-input-field' } } = form.text_field :title, required: true, aria: { required: true }, maxlength: 255, autofocus: true, autocomplete: 'off', class: 'form-control pad', dir: 'auto', data: { qa_selector: 'issuable_form_title_field' } - if issuable.respond_to?(:draft?) - .form-text.text-muted - .js-wip-explanation{ style: "display: none;" } - = remove_wip_text - .js-no-wip-explanation - - if has_wip_commits - = _('It looks like you have some draft commits in this branch.') - %br - .invisible - .js-unwrap-on-load - = add_wip_text + .gl-pt-3 + = render Pajamas::CheckboxTagComponent.new(name: 'mark_as_draft', checkbox_options: { class: 'js-toggle-draft' }) do |c| + = c.label do + = s_('MergeRequests|Mark as draft') + = c.help_text do + = s_('MergeRequests|Drafts cannot be merged until marked ready.') - if no_issuable_templates && can?(current_user, :push_code, issuable.project) = render 'shared/issuable/form/default_templates' -- GitLab From b88f3f20f9f3c012a21c8fc03a75c96eec1d54a9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 4 Jan 2023 09:38:57 -0700 Subject: [PATCH 02/12] Update localization parts --- locale/gitlab.pot | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 22152508389b17..a69905c7331231 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -823,12 +823,6 @@ msgstr "" msgid "%{linkStart} Learn more%{linkEnd}." msgstr "" -msgid "%{link_start}Remove the %{draft_snippet} prefix%{link_end} from the title to allow this merge request to be merged when it's ready." -msgstr "" - -msgid "%{link_start}Start the title with %{draft_snippet}%{link_end} to prevent a merge request draft from merging before it's ready." -msgstr "" - msgid "%{listToShow}, and %{awardsListLength} more" msgstr "" @@ -23414,9 +23408,6 @@ msgstr "" msgid "Issue|Title" msgstr "" -msgid "It looks like you have some draft commits in this branch." -msgstr "" - msgid "It looks like you're attempting to activate your subscription. Use %{a_start}the Subscription page%{a_end} instead." msgstr "" @@ -26314,6 +26305,12 @@ msgstr "" msgid "MergeRequests|Create issue to resolve thread" msgstr "" +msgid "MergeRequests|Drafts cannot be merged until marked ready." +msgstr "" + +msgid "MergeRequests|Mark as draft" +msgstr "" + msgid "MergeRequests|Reference copied" msgstr "" -- GitLab From 3a791cee2267e35451d61a526768933a5e9c7a12 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 5 Jan 2023 10:30:51 -0700 Subject: [PATCH 03/12] Update the mark as draft docs --- doc/user/project/merge_requests/drafts.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/doc/user/project/merge_requests/drafts.md b/doc/user/project/merge_requests/drafts.md index 0bc9b337e3b2b2..2a6dd70397bca7 100644 --- a/doc/user/project/merge_requests/drafts.md +++ b/doc/user/project/merge_requests/drafts.md @@ -21,12 +21,13 @@ the **Merge** button until you remove the **Draft** flag: > - [Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/228685) all support for using **WIP** in GitLab 14.8. > - **Mark as draft** and **Mark as ready** buttons [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/227421) in GitLab 13.5. > `/draft` quick action as a toggle [deprecated](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92654) in GitLab 15.4. +> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108073) in GitLab 15.8, toggling the draft status now uses a checkbox rather than text buttons. There are several ways to flag a merge request as a draft: - **Viewing a merge request**: In the top right corner of the merge request, select **Mark as draft**. - **Creating or editing a merge request**: Add `[Draft]`, `Draft:` or `(Draft)` to - the beginning of the merge request's title, or select **Start the title with Draft:** + the beginning of the merge request's title, or check **Mark as draft** below the **Title** field. - **Commenting in an existing merge request**: Add the `/draft` [quick action](../quick_actions.md#issues-merge-requests-and-epics) @@ -47,7 +48,7 @@ When a merge request is ready to be merged, you can remove the `Draft` flag in s ![Mark as ready](img/draft_blocked_merge_button_v13_10.png) - **Editing an existing merge request**: Remove `[Draft]`, `Draft:` or `(Draft)` - from the beginning of the title, or select **Remove the Draft: prefix from the title** + from the beginning of the title, or uncheck **Mark as draft** below the **Title** field. - **Commenting in an existing merge request**: Add the `/ready` [quick action](../quick_actions.md#issues-merge-requests-and-epics) -- GitLab From 245eb7e0e10c2024e6e088f28e6d813a58a27f95 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 5 Jan 2023 12:00:15 -0700 Subject: [PATCH 04/12] Test new behavior of the issuable form --- .../frontend/issuable/issuable_form_spec.js | 8 +- spec/frontend/issuable/helpers.js | 18 ++++ spec/frontend/issuable/issuable_form_spec.js | 92 ++++++++++++++++--- 3 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 spec/frontend/issuable/helpers.js diff --git a/ee/spec/frontend/issuable/issuable_form_spec.js b/ee/spec/frontend/issuable/issuable_form_spec.js index 63e3f1f46cbcbe..6c31bfd89e963c 100644 --- a/ee/spec/frontend/issuable/issuable_form_spec.js +++ b/ee/spec/frontend/issuable/issuable_form_spec.js @@ -4,6 +4,8 @@ import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; import IssuableForm from 'ee/issuable/issuable_form'; import IssuableFormCE from '~/issuable/issuable_form'; +import { getSaveableFormChildren } from '../../../../spec/frontend/issuable/helpers'; + jest.mock('~/autosave'); const createIssuable = (form) => { @@ -17,6 +19,7 @@ describe('IssuableForm', () => { setHTMLFixture(`
+
`); @@ -38,10 +41,11 @@ describe('IssuableForm', () => { it('creates weight autosave when weight input exist', () => { $form.append(''); const $weight = $form.find('input[name*="[weight]"]'); - const totalAutosaveFormFields = $form.children().length; createIssuable($form); - expect(Autosave).toHaveBeenCalledTimes(totalAutosaveFormFields); + const children = getSaveableFormChildren($form[0]); + + expect(Autosave).toHaveBeenCalledTimes(children.length); expect(Autosave).toHaveBeenLastCalledWith( $weight.get(0), ['/', '', 'weight'], diff --git a/spec/frontend/issuable/helpers.js b/spec/frontend/issuable/helpers.js new file mode 100644 index 00000000000000..632d69c2c88b55 --- /dev/null +++ b/spec/frontend/issuable/helpers.js @@ -0,0 +1,18 @@ +export function getSaveableFormChildren(form, exclude = ['input.js-toggle-draft']) { + const children = Array.from(form.children); + const saveable = children.filter((e) => { + const isFiltered = exclude.reduce( + ({ isFiltered: filtered, element }, selector) => { + return { + isFiltered: filtered || element.matches(selector), + element, + }; + }, + { isFiltered: false, element: e }, + ); + + return !isFiltered.isFiltered; + }); + + return saveable; +} diff --git a/spec/frontend/issuable/issuable_form_spec.js b/spec/frontend/issuable/issuable_form_spec.js index 28ec0e22d8bfcf..a228a107a3eae1 100644 --- a/spec/frontend/issuable/issuable_form_spec.js +++ b/spec/frontend/issuable/issuable_form_spec.js @@ -4,6 +4,8 @@ import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; import IssuableForm from '~/issuable/issuable_form'; import setWindowLocation from 'helpers/set_window_location_helper'; +import { getSaveableFormChildren } from './helpers'; + jest.mock('~/autosave'); const createIssuable = (form) => { @@ -18,6 +20,7 @@ describe('IssuableForm', () => { setHTMLFixture(`
+
`); @@ -99,10 +102,11 @@ describe('IssuableForm', () => { ])('creates $id autosave when $id input exist', ({ id, input, selector }) => { $form.append(input); const $input = $form.find(selector); - const totalAutosaveFormFields = $form.children().length; createIssuable($form); - expect(Autosave).toHaveBeenCalledTimes(totalAutosaveFormFields); + const children = getSaveableFormChildren($form[0]); + + expect(Autosave).toHaveBeenCalledTimes(children.length); expect(Autosave).toHaveBeenLastCalledWith( $input.get(0), ['/', '', id], @@ -153,12 +157,17 @@ describe('IssuableForm', () => { }); }); - describe('wip', () => { + describe('draft', () => { + let titleField; + let toggleDraft; + beforeEach(() => { instance = createIssuable($form); + titleField = document.querySelector('input[name="[title]"]'); + toggleDraft = document.querySelector('input.js-toggle-draft'); }); - describe('removeWip', () => { + describe('removeDraft', () => { it.each` prefix ${'draFT: '} @@ -169,25 +178,25 @@ describe('IssuableForm', () => { ${' (DrafT)'} ${'draft: [draft] (draft)'} `('removes "$prefix" from the beginning of the title', ({ prefix }) => { - instance.titleField.val(`${prefix}The Issuable's Title Value`); + titleField.value = `${prefix}The Issuable's Title Value`; - instance.removeWip(); + instance.removeDraft(); - expect(instance.titleField.val()).toBe("The Issuable's Title Value"); + expect(titleField.value).toBe("The Issuable's Title Value"); }); }); - describe('addWip', () => { + describe('addDraft', () => { it("properly adds the work in progress prefix to the Issuable's title", () => { - instance.titleField.val("The Issuable's Title Value"); + titleField.value = "The Issuable's Title Value"; - instance.addWip(); + instance.addDraft(); - expect(instance.titleField.val()).toBe("Draft: The Issuable's Title Value"); + expect(titleField.value).toBe("Draft: The Issuable's Title Value"); }); }); - describe('workInProgress', () => { + describe('isMarkedDraft', () => { it.each` title | expected ${'draFT: something is happening'} | ${true} @@ -195,9 +204,64 @@ describe('IssuableForm', () => { ${'something is happening to drafts'} | ${false} ${'something is happening'} | ${false} `('returns $expected with "$title"', ({ title, expected }) => { - instance.titleField.val(title); + titleField.value = title; + + expect(instance.isMarkedDraft()).toBe(expected); + }); + }); + + describe('readDraftStatus', () => { + it.each` + title | checked + ${'Draft: my title'} | ${true} + ${'my title'} | ${false} + `( + 'sets the draft checkbox checked status to $checked when the title is $title', + ({ title, checked }) => { + titleField.value = title; + + instance.readDraftStatus(); + + expect(toggleDraft.checked).toBe(checked); + }, + ); + }); + + describe('writeDraftStatus', () => { + it.each` + checked | title + ${true} | ${'Draft: my title'} + ${false} | ${'my title'} + `( + 'updates the title to $title when the draft checkbox checked status is $checked', + ({ checked, title }) => { + titleField.value = 'my title'; + toggleDraft.checked = checked; + + instance.writeDraftStatus(); + + expect(titleField.value).toBe(title); + }, + ); + }); + + describe('uncheckDraft', () => { + it('removes the check from the draft toggle checkbox', () => { + toggleDraft.checked = true; + + instance.uncheckDraft(); + + expect(toggleDraft.checked).toBe(false); + }); + }); + + describe('checkDraft', () => { + it('adds the check to the draft toggle checkbox', () => { + toggleDraft.checked = false; + + instance.checkDraft(); - expect(instance.workInProgress()).toBe(expected); + expect(toggleDraft.checked).toBe(true); }); }); }); -- GitLab From 896e8c8c95def49f75131b4f1a743a17728b62d1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 9 Jan 2023 12:10:25 -0700 Subject: [PATCH 05/12] Remove the draft management code on non-MR pages ... like Issues --- app/assets/javascripts/issuable/issuable_form.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/issuable/issuable_form.js b/app/assets/javascripts/issuable/issuable_form.js index c885d0367fa0ad..655cc82bba9185 100644 --- a/app/assets/javascripts/issuable/issuable_form.js +++ b/app/assets/javascripts/issuable/issuable_form.js @@ -159,10 +159,12 @@ export default class IssuableForm { } initDraft() { - this.form.on('click', '.js-toggle-draft', () => this.writeDraftStatus()); - this.titleField.on('keyup blur', () => this.readDraftStatus()); + if (this.draftCheck) { + this.form.on('click', '.js-toggle-draft', () => this.writeDraftStatus()); + this.titleField.on('keyup blur', () => this.readDraftStatus()); - this.readDraftStatus(); + this.readDraftStatus(); + } } isMarkedDraft() { -- GitLab From 3d4d7a3f869577861bda629f511a3e5fec8d0271 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 9 Jan 2023 13:14:57 -0700 Subject: [PATCH 06/12] Update RSpec tests with new UI --- .../user_sees_wip_help_message_spec.rb | 28 ++++--------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/spec/features/merge_request/user_sees_wip_help_message_spec.rb b/spec/features/merge_request/user_sees_wip_help_message_spec.rb index fdefe5ffb06dff..f4d1957d63457d 100644 --- a/spec/features/merge_request/user_sees_wip_help_message_spec.rb +++ b/spec/features/merge_request/user_sees_wip_help_message_spec.rb @@ -12,7 +12,7 @@ end context 'with draft commits' do - it 'shows a specific draft hint' do + it 'shows the draft toggle' do visit project_new_merge_request_path( project, merge_request: { @@ -22,16 +22,13 @@ target_branch: 'master' }) - within_wip_explanation do - expect(page).to have_text( - 'It looks like you have some draft commits in this branch' - ) - end + expect(page).to have_text('Mark as draft') + expect(page).to have_text('Drafts cannot be merged until marked ready.') end end context 'without draft commits' do - it 'shows the regular draft message' do + it 'shows the draft toggle' do visit project_new_merge_request_path( project, merge_request: { @@ -41,21 +38,8 @@ target_branch: 'master' }) - within_wip_explanation do - expect(page).not_to have_text( - 'It looks like you have some draft commits in this branch' - ) - expect(page).to have_text( - "Start the title with Draft: to prevent a merge request draft \ -from merging before it's ready." - ) - end - end - end - - def within_wip_explanation(&block) - page.within '.js-no-wip-explanation' do - yield + expect(page).to have_text('Mark as draft') + expect(page).to have_text('Drafts cannot be merged until marked ready.') end end end -- GitLab From 7b46ddd44027a77df1f875a0d43bc24bc705a0ae Mon Sep 17 00:00:00 2001 From: Amy Qualls Date: Tue, 10 Jan 2023 23:39:14 +0000 Subject: [PATCH 07/12] Apply TW docs improvements --- doc/user/project/merge_requests/drafts.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/user/project/merge_requests/drafts.md b/doc/user/project/merge_requests/drafts.md index 2a6dd70397bca7..c216514fff498a 100644 --- a/doc/user/project/merge_requests/drafts.md +++ b/doc/user/project/merge_requests/drafts.md @@ -21,13 +21,13 @@ the **Merge** button until you remove the **Draft** flag: > - [Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/228685) all support for using **WIP** in GitLab 14.8. > - **Mark as draft** and **Mark as ready** buttons [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/227421) in GitLab 13.5. > `/draft` quick action as a toggle [deprecated](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92654) in GitLab 15.4. -> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108073) in GitLab 15.8, toggling the draft status now uses a checkbox rather than text buttons. +> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108073) the draft status to use a checkbox in GitLab 15.8. There are several ways to flag a merge request as a draft: - **Viewing a merge request**: In the top right corner of the merge request, select **Mark as draft**. - **Creating or editing a merge request**: Add `[Draft]`, `Draft:` or `(Draft)` to - the beginning of the merge request's title, or check **Mark as draft** + the beginning of the merge request's title, or select **Mark as draft** below the **Title** field. - **Commenting in an existing merge request**: Add the `/draft` [quick action](../quick_actions.md#issues-merge-requests-and-epics) @@ -48,7 +48,7 @@ When a merge request is ready to be merged, you can remove the `Draft` flag in s ![Mark as ready](img/draft_blocked_merge_button_v13_10.png) - **Editing an existing merge request**: Remove `[Draft]`, `Draft:` or `(Draft)` - from the beginning of the title, or uncheck **Mark as draft** + from the beginning of the title, or clear **Mark as draft** below the **Title** field. - **Commenting in an existing merge request**: Add the `/ready` [quick action](../quick_actions.md#issues-merge-requests-and-epics) -- GitLab From 6e777697fb29eb7d3d00775e4bcd9c19d33f042f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 11 Jan 2023 10:21:32 -0700 Subject: [PATCH 08/12] Make RSpec tests explicitly about the checkbox - Name of file - Test for the checkbox itself --- ...help_message_spec.rb => user_can_see_draft_toggle_spec.rb} | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) rename spec/features/merge_request/{user_sees_wip_help_message_spec.rb => user_can_see_draft_toggle_spec.rb} (81%) diff --git a/spec/features/merge_request/user_sees_wip_help_message_spec.rb b/spec/features/merge_request/user_can_see_draft_toggle_spec.rb similarity index 81% rename from spec/features/merge_request/user_sees_wip_help_message_spec.rb rename to spec/features/merge_request/user_can_see_draft_toggle_spec.rb index f4d1957d63457d..78864e9f5a3dc1 100644 --- a/spec/features/merge_request/user_sees_wip_help_message_spec.rb +++ b/spec/features/merge_request/user_can_see_draft_toggle_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe 'Merge request > User sees draft help message', feature_category: :code_review_workflow do +RSpec.describe 'Merge request > User sees draft toggle', feature_category: :code_review do let(:project) { create(:project, :public, :repository) } let(:user) { project.creator } @@ -22,6 +22,7 @@ target_branch: 'master' }) + expect(page).to have_css('input[type="checkbox"].js-toggle-draft', count: 1) expect(page).to have_text('Mark as draft') expect(page).to have_text('Drafts cannot be merged until marked ready.') end @@ -38,6 +39,7 @@ target_branch: 'master' }) + expect(page).to have_css('input[type="checkbox"].js-toggle-draft', count: 1) expect(page).to have_text('Mark as draft') expect(page).to have_text('Drafts cannot be merged until marked ready.') end -- GitLab From d51d7925974569ee93934294f197a7dd521d90d7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 11 Jan 2023 10:33:12 -0700 Subject: [PATCH 09/12] Update with notes from MR review In essence, this bakes the existing UI into what the code can do. --- .../javascripts/issuable/issuable_form.js | 14 ++----------- spec/frontend/issuable/issuable_form_spec.js | 20 ------------------- 2 files changed, 2 insertions(+), 32 deletions(-) diff --git a/app/assets/javascripts/issuable/issuable_form.js b/app/assets/javascripts/issuable/issuable_form.js index 655cc82bba9185..6a857bc4945fb8 100644 --- a/app/assets/javascripts/issuable/issuable_form.js +++ b/app/assets/javascripts/issuable/issuable_form.js @@ -160,7 +160,7 @@ export default class IssuableForm { initDraft() { if (this.draftCheck) { - this.form.on('click', '.js-toggle-draft', () => this.writeDraftStatus()); + this.form.on('click', this.draftCheck, () => this.writeDraftStatus()); this.titleField.on('keyup blur', () => this.readDraftStatus()); this.readDraftStatus(); @@ -171,11 +171,7 @@ export default class IssuableForm { return this.draftRegex.test(this.titleField.val()); } readDraftStatus() { - if (this.isMarkedDraft()) { - this.checkDraft(); - } else { - this.uncheckDraft(); - } + this.draftCheck.checked = this.isMarkedDraft(); } writeDraftStatus() { if (this.draftCheck.checked) { @@ -188,14 +184,8 @@ export default class IssuableForm { removeDraft() { return this.titleField.val(this.titleField.val().replace(this.draftRegex, '')); } - uncheckDraft() { - this.draftCheck.checked = false; - } addDraft() { this.titleField.val(`Draft: ${this.titleField.val()}`); } - checkDraft() { - this.draftCheck.checked = true; - } } diff --git a/spec/frontend/issuable/issuable_form_spec.js b/spec/frontend/issuable/issuable_form_spec.js index a228a107a3eae1..3e778e50fb851d 100644 --- a/spec/frontend/issuable/issuable_form_spec.js +++ b/spec/frontend/issuable/issuable_form_spec.js @@ -244,25 +244,5 @@ describe('IssuableForm', () => { }, ); }); - - describe('uncheckDraft', () => { - it('removes the check from the draft toggle checkbox', () => { - toggleDraft.checked = true; - - instance.uncheckDraft(); - - expect(toggleDraft.checked).toBe(false); - }); - }); - - describe('checkDraft', () => { - it('adds the check to the draft toggle checkbox', () => { - toggleDraft.checked = false; - - instance.checkDraft(); - - expect(toggleDraft.checked).toBe(true); - }); - }); }); }); -- GitLab From b8f8f699faa93ac72cec98103b00f58bb4e78587 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 11 Jan 2023 10:34:26 -0700 Subject: [PATCH 10/12] Update test with new helper since dangerbot is complaining now --- spec/features/merge_request/user_can_see_draft_toggle_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/merge_request/user_can_see_draft_toggle_spec.rb b/spec/features/merge_request/user_can_see_draft_toggle_spec.rb index 78864e9f5a3dc1..4d6c174e0e0942 100644 --- a/spec/features/merge_request/user_can_see_draft_toggle_spec.rb +++ b/spec/features/merge_request/user_can_see_draft_toggle_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe 'Merge request > User sees draft toggle', feature_category: :code_review do - let(:project) { create(:project, :public, :repository) } + let_it_be(:project) { create(:project, :public, :repository) } let(:user) { project.creator } before do -- GitLab From b32fe8476eeb476acc923a11a3cdc722481e09bb Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 11 Jan 2023 10:56:23 -0700 Subject: [PATCH 11/12] Remove existing spacing that rubocop is now complaining about --- spec/features/merge_request/user_can_see_draft_toggle_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/merge_request/user_can_see_draft_toggle_spec.rb b/spec/features/merge_request/user_can_see_draft_toggle_spec.rb index 4d6c174e0e0942..0282c954225b09 100644 --- a/spec/features/merge_request/user_can_see_draft_toggle_spec.rb +++ b/spec/features/merge_request/user_can_see_draft_toggle_spec.rb @@ -4,7 +4,7 @@ RSpec.describe 'Merge request > User sees draft toggle', feature_category: :code_review do let_it_be(:project) { create(:project, :public, :repository) } - let(:user) { project.creator } + let(:user) { project.creator } before do project.add_maintainer(user) -- GitLab From 0346192abd8de2d8d765de63eac8c6e3029e4a3d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 16 Jan 2023 14:04:42 -0700 Subject: [PATCH 12/12] JQuery's `.on` does not accept an element as the second parameter In any case, let's not use jQuery unless we absolutely must. And, in many cases, binding listeners to parent elements causes more issues that it solves. For example: if you bind to a parent element and don't filter the caught events, any click anywhere will trigger the handler. It's good to bind to parents in the case of repeated rendering so that the bound listeners don't increase linearly with DOM size updates, but in the case of a one-off - like here - it's a bit redundant. Reverts suggestion https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108073#note_1235661501 implemented in d51d7925974569ee93934294f197a7dd521d90d7 --- app/assets/javascripts/issuable/issuable_form.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/issuable/issuable_form.js b/app/assets/javascripts/issuable/issuable_form.js index 6a857bc4945fb8..8a094d5d6882d0 100644 --- a/app/assets/javascripts/issuable/issuable_form.js +++ b/app/assets/javascripts/issuable/issuable_form.js @@ -160,7 +160,7 @@ export default class IssuableForm { initDraft() { if (this.draftCheck) { - this.form.on('click', this.draftCheck, () => this.writeDraftStatus()); + this.draftCheck.addEventListener('click', () => this.writeDraftStatus()); this.titleField.on('keyup blur', () => this.readDraftStatus()); this.readDraftStatus(); -- GitLab