From 1e653018ba664dc2ee596e1025e77677df1b839d Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Thu, 19 Jan 2023 11:58:46 +0000
Subject: [PATCH] Switch draft state toggle to use a checkbox
Changelog: changed
---
.../javascripts/issuable/issuable_form.js | 50 +++++--------
.../shared/issuable/form/_title.html.haml | 21 ++----
doc/user/project/merge_requests/drafts.md | 5 +-
.../frontend/issuable/issuable_form_spec.js | 8 ++-
locale/gitlab.pot | 15 ++--
.../user_can_see_draft_toggle_spec.rb | 47 ++++++++++++
.../user_sees_wip_help_message_spec.rb | 61 ----------------
spec/frontend/issuable/helpers.js | 18 +++++
spec/frontend/issuable/issuable_form_spec.js | 72 +++++++++++++++----
9 files changed, 161 insertions(+), 136 deletions(-)
create mode 100644 spec/features/merge_request/user_can_see_draft_toggle_spec.rb
delete mode 100644 spec/features/merge_request/user_sees_wip_help_message_spec.rb
create mode 100644 spec/frontend/issuable/helpers.js
diff --git a/app/assets/javascripts/issuable/issuable_form.js b/app/assets/javascripts/issuable/issuable_form.js
index 99a3f76ca764d..8a094d5d6882d 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,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.draftCheck.addEventListener('click', () => 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()}`);
}
}
diff --git a/app/views/shared/issuable/form/_title.html.haml b/app/views/shared/issuable/form/_title.html.haml
index 0f6ef33d5324e..4d31baee25b0e 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 = '<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'
diff --git a/doc/user/project/merge_requests/drafts.md b/doc/user/project/merge_requests/drafts.md
index 0bc9b337e3b2b..c216514fff498 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) 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
- **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)
diff --git a/ee/spec/frontend/issuable/issuable_form_spec.js b/ee/spec/frontend/issuable/issuable_form_spec.js
index 63e3f1f46cbcb..6c31bfd89e963 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(`
<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'],
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 22152508389b1..a69905c733123 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 ""
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
new file mode 100644
index 0000000000000..0282c954225b0
--- /dev/null
+++ b/spec/features/merge_request/user_can_see_draft_toggle_spec.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+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)
+ sign_in(user)
+ end
+
+ context 'with draft commits' do
+ it 'shows the draft toggle' do
+ visit project_new_merge_request_path(
+ project,
+ merge_request: {
+ source_project_id: project.id,
+ target_project_id: project.id,
+ source_branch: 'wip',
+ 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
+ end
+
+ context 'without draft commits' do
+ it 'shows the draft toggle' do
+ visit project_new_merge_request_path(
+ project,
+ merge_request: {
+ source_project_id: project.id,
+ target_project_id: project.id,
+ source_branch: 'fix',
+ 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
+ end
+end
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
deleted file mode 100644
index fdefe5ffb06df..0000000000000
--- a/spec/features/merge_request/user_sees_wip_help_message_spec.rb
+++ /dev/null
@@ -1,61 +0,0 @@
-# frozen_string_literal: true
-
-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 }
-
- before do
- project.add_maintainer(user)
- sign_in(user)
- end
-
- context 'with draft commits' do
- it 'shows a specific draft hint' do
- visit project_new_merge_request_path(
- project,
- merge_request: {
- source_project_id: project.id,
- target_project_id: project.id,
- source_branch: 'wip',
- target_branch: 'master'
- })
-
- within_wip_explanation do
- expect(page).to have_text(
- 'It looks like you have some draft commits in this branch'
- )
- end
- end
- end
-
- context 'without draft commits' do
- it 'shows the regular draft message' do
- visit project_new_merge_request_path(
- project,
- merge_request: {
- source_project_id: project.id,
- target_project_id: project.id,
- source_branch: 'fix',
- 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
- end
- end
-end
diff --git a/spec/frontend/issuable/helpers.js b/spec/frontend/issuable/helpers.js
new file mode 100644
index 0000000000000..632d69c2c88b5
--- /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 28ec0e22d8bfc..3e778e50fb851 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(`
<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);
+ },
+ );
+ });
});
});
--
GitLab