diff --git a/app/assets/javascripts/issuable/issuable_form.js b/app/assets/javascripts/issuable/issuable_form.js index 6a857bc4945fb8e34ccd92187f40c57093b5f2ca..99a3f76ca764d5a55ce84550be15ef764bc7a659 100644 --- a/app/assets/javascripts/issuable/issuable_form.js +++ b/app/assets/javascripts/issuable/issuable_form.js @@ -60,6 +60,8 @@ 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 @@ -84,7 +86,6 @@ 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; } @@ -92,7 +93,8 @@ 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.initDraft(); + this.form.find('.js-unwrap-on-load').unwrap(); + this.initWip(); const $issuableDueDate = $('#issuable-due-date'); @@ -158,34 +160,48 @@ export default class IssuableForm { }); } - initDraft() { - if (this.draftCheck) { - this.form.on('click', this.draftCheck, () => this.writeDraftStatus()); - this.titleField.on('keyup blur', () => this.readDraftStatus()); - - this.readDraftStatus(); + 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(); } - isMarkedDraft() { + workInProgress() { return this.draftRegex.test(this.titleField.val()); } - readDraftStatus() { - this.draftCheck.checked = this.isMarkedDraft(); + + 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(); } - writeDraftStatus() { - if (this.draftCheck.checked) { - this.addDraft(); + + toggleWip(event) { + event.preventDefault(); + if (this.workInProgress()) { + this.removeWip(); } else { - this.removeDraft(); + this.addWip(); } + return this.renderWipExplanation(); } - removeDraft() { + removeWip() { return this.titleField.val(this.titleField.val().replace(this.draftRegex, '')); } - addDraft() { + addWip() { 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 4d31baee25b0e42def19d2e974fc7728f7befd45..0f6ef33d5324eee01e24d87028d183283110eaf4 100644 --- a/app/views/shared/issuable/form/_title.html.haml +++ b/app/views/shared/issuable/form/_title.html.haml @@ -1,18 +1,27 @@ - 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?) - .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.') + .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 - 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 c216514fff498a5eb5b2786569064a01ce4d48ad..0bc9b337e3b2b205c768c1dcc3008d21a171421b 100644 --- a/doc/user/project/merge_requests/drafts.md +++ b/doc/user/project/merge_requests/drafts.md @@ -21,13 +21,12 @@ 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 **Mark as draft** + the beginning of the merge request's title, or select **Start the title with 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 +47,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 clear **Mark as draft** + from the beginning of the title, or select **Remove the Draft: prefix from the title** 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 6c31bfd89e963c9a5518bca639a917d11a4a66bc..63e3f1f46cbcbece06aab4104ff2b08284f7f8a0 100644 --- a/ee/spec/frontend/issuable/issuable_form_spec.js +++ b/ee/spec/frontend/issuable/issuable_form_spec.js @@ -4,8 +4,6 @@ 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) => { @@ -19,7 +17,6 @@ describe('IssuableForm', () => { setHTMLFixture(` <form> <input name="[title]" /> - <input type="checkbox" class="js-toggle-draft" /> <textarea name="[description]"></textarea> </form> `); @@ -41,11 +38,10 @@ 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); - const children = getSaveableFormChildren($form[0]); - - expect(Autosave).toHaveBeenCalledTimes(children.length); + expect(Autosave).toHaveBeenCalledTimes(totalAutosaveFormFields); expect(Autosave).toHaveBeenLastCalledWith( $weight.get(0), ['/', '', 'weight'], diff --git a/locale/gitlab.pot b/locale/gitlab.pot index da7029d383b62181031a1978b56e3b61422b78e0..6aadbddd7ef283ade9e483e6cbac6303e5122e8b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -823,6 +823,12 @@ 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 "" @@ -23411,6 +23417,9 @@ 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 "" @@ -26320,12 +26329,6 @@ 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 deleted file mode 100644 index 0282c954225b093a1de247b2bc676a08500f9de8..0000000000000000000000000000000000000000 --- a/spec/features/merge_request/user_can_see_draft_toggle_spec.rb +++ /dev/null @@ -1,47 +0,0 @@ -# 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 new file mode 100644 index 0000000000000000000000000000000000000000..fdefe5ffb06dff925a2aa0b61dbc3a5e2c3e2fda --- /dev/null +++ b/spec/features/merge_request/user_sees_wip_help_message_spec.rb @@ -0,0 +1,61 @@ +# 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 deleted file mode 100644 index 632d69c2c88b5558d2a4d037bd6c454a006fc02b..0000000000000000000000000000000000000000 --- a/spec/frontend/issuable/helpers.js +++ /dev/null @@ -1,18 +0,0 @@ -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 3e778e50fb851dde347ce73f1e35bce874fd414c..28ec0e22d8bfcf1f2b54743604421856036fe472 100644 --- a/spec/frontend/issuable/issuable_form_spec.js +++ b/spec/frontend/issuable/issuable_form_spec.js @@ -4,8 +4,6 @@ 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) => { @@ -20,7 +18,6 @@ describe('IssuableForm', () => { setHTMLFixture(` <form> <input name="[title]" /> - <input type="checkbox" class="js-toggle-draft" /> <textarea name="[description]"></textarea> </form> `); @@ -102,11 +99,10 @@ 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); - const children = getSaveableFormChildren($form[0]); - - expect(Autosave).toHaveBeenCalledTimes(children.length); + expect(Autosave).toHaveBeenCalledTimes(totalAutosaveFormFields); expect(Autosave).toHaveBeenLastCalledWith( $input.get(0), ['/', '', id], @@ -157,17 +153,12 @@ describe('IssuableForm', () => { }); }); - describe('draft', () => { - let titleField; - let toggleDraft; - + describe('wip', () => { beforeEach(() => { instance = createIssuable($form); - titleField = document.querySelector('input[name="[title]"]'); - toggleDraft = document.querySelector('input.js-toggle-draft'); }); - describe('removeDraft', () => { + describe('removeWip', () => { it.each` prefix ${'draFT: '} @@ -178,25 +169,25 @@ describe('IssuableForm', () => { ${' (DrafT)'} ${'draft: [draft] (draft)'} `('removes "$prefix" from the beginning of the title', ({ prefix }) => { - titleField.value = `${prefix}The Issuable's Title Value`; + instance.titleField.val(`${prefix}The Issuable's Title Value`); - instance.removeDraft(); + instance.removeWip(); - expect(titleField.value).toBe("The Issuable's Title Value"); + expect(instance.titleField.val()).toBe("The Issuable's Title Value"); }); }); - describe('addDraft', () => { + describe('addWip', () => { it("properly adds the work in progress prefix to the Issuable's title", () => { - titleField.value = "The Issuable's Title Value"; + instance.titleField.val("The Issuable's Title Value"); - instance.addDraft(); + instance.addWip(); - expect(titleField.value).toBe("Draft: The Issuable's Title Value"); + expect(instance.titleField.val()).toBe("Draft: The Issuable's Title Value"); }); }); - describe('isMarkedDraft', () => { + describe('workInProgress', () => { it.each` title | expected ${'draFT: something is happening'} | ${true} @@ -204,45 +195,10 @@ describe('IssuableForm', () => { ${'something is happening to drafts'} | ${false} ${'something is happening'} | ${false} `('returns $expected with "$title"', ({ title, expected }) => { - titleField.value = title; + instance.titleField.val(title); - expect(instance.isMarkedDraft()).toBe(expected); + expect(instance.workInProgress()).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); - }, - ); - }); }); });