Switch draft state toggle to use a checkbox

Changelog: changed

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,34 @@ 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;
+  initDraft() {
+    if (this.draftCheck) {
+      this.form.on('click', this.draftCheck, () => this.writeDraftStatus());
+      this.titleField.on('keyup blur', () => this.readDraftStatus());
+
+      this.readDraftStatus();
     }
-    this.form.on('click', '.js-toggle-wip', this.toggleWip);
-    this.titleField.on('keyup blur', this.renderWipExplanation);
-    return this.renderWipExplanation();
   }
 
-  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();
-    }
-    this.$wipExplanation.hide();
-    return this.$noWipExplanation.show();
+  readDraftStatus() {
+    this.draftCheck.checked = this.isMarkedDraft();
   }
-
-  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, ''));
   }
 
-  addWip() {
+  addDraft() {
     this.titleField.val(`Draft: ${this.titleField.val()}`);
   }
 }

app/views/shared/issuable/form/_title.html.haml

 - 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 = '<a href="" class="js-toggle-wip">'
-- toggle_wip_link_end = '</a>'
-- 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: '<code>Draft:</code>'.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: '<code>Draft</code>'.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'

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) 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 select **Start the title with 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)
@@ -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 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)

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(`
       <form>
         <input name="[title]" />
+        <input type="checkbox" class="js-toggle-draft" />
         <textarea name="[description]"></textarea>
       </form>
     `);
@@ -38,10 +41,11 @@ describe('IssuableForm', () => {
     it('creates weight autosave when weight input exist', () => {
       $form.append('<input name="[weight]" />');
       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'],

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 ""
@@ -23360,9 +23354,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 ""
@@ -26263,6 +26254,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 ""

spec/features/merge_request/user_sees_wip_help_message_spec.rb → spec/features/merge_request/user_can_see_draft_toggle_spec.rb

@@ -2,9 +2,9 @@
 
 require 'spec_helper'
 
-RSpec.describe 'Merge request > User sees draft help message', feature_category: :code_review_workflow do
-  let(:project) { create(:project, :public, :repository) }
-  let(:user)    { project.creator }
+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 }
 
   before do
     project.add_maintainer(user)
@@ -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,14 @@
           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_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
   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 +39,9 @@
           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_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
   end
 end

spec/frontend/issuable/helpers.js

+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;
+}

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(`
       <form>
         <input name="[title]" />
+        <input type="checkbox" class="js-toggle-draft" />
         <textarea name="[description]"></textarea>
       </form>
     `);
@@ -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,10 +204,45 @@ 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.workInProgress()).toBe(expected);
+        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);
+        },
+      );
+    });
   });
 });