Merge branch '241058-mg-update-reply-placeholder' into 'master'

Replace styled discussion reply button with textarea

See merge request gitlab-org/gitlab!54380

app/assets/javascripts/design_management/components/design_notes/design_discussion.vue

@@ -257,8 +257,8 @@ export default {
         <reply-placeholder
           v-if="!isFormVisible"
           class="qa-discussion-reply"
-          :button-text="__('Reply...')"
-          @onClick="showForm"
+          :placeholder-text="__('Reply…')"
+          @focus="showForm"
         />
         <apollo-mutation
           v-else

app/assets/javascripts/diffs/components/diff_discussion_reply.vue

@@ -35,8 +35,9 @@ export default {
       <slot v-if="hasForm" name="form"></slot>
       <template v-else-if="renderReplyPlaceholder">
         <reply-placeholder
-          :button-text="__('Start a new discussion...')"
-          @onClick="$emit('showNewDiscussionForm')"
+          :placeholder-text="__('Start a new discussion…')"
+          :label-text="__('New discussion')"
+          @focus="$emit('showNewDiscussionForm')"
         />
       </template>
     </template>

app/assets/javascripts/notes/components/discussion_actions.vue

@@ -50,8 +50,8 @@ export default {
   <div class="discussion-with-resolve-btn clearfix">
     <reply-placeholder
       data-qa-selector="discussion_reply_tab"
-      :button-text="s__('MergeRequests|Reply...')"
-      @onClick="$emit('showReplyForm')"
+      :placeholder-text="__('Reply…')"
+      @focus="$emit('showReplyForm')"
     />
 
     <div v-if="userCanResolveDiscussion" class="btn-group discussion-actions" role="group">

app/assets/javascripts/notes/components/discussion_reply_placeholder.vue

 <script>
+import { __ } from '~/locale';
+
 export default {
   name: 'ReplyPlaceholder',
   props: {
-    buttonText: {
+    placeholderText: {
+      type: String,
+      required: false,
+      default: __('Reply…'),
+    },
+    labelText: {
       type: String,
-      required: true,
+      required: false,
+      default: __('Reply to comment'),
     },
   },
 };
 </script>
 
 <template>
-  <button
-    ref="button"
-    type="button"
-    class="js-vue-discussion-reply btn btn-text-field"
-    :title="s__('MergeRequests|Add a reply')"
-    @click="$emit('onClick')"
-  >
-    {{ buttonText }}
-  </button>
+  <textarea
+    ref="textarea"
+    rows="1"
+    class="reply-placeholder-text-field js-vue-discussion-reply"
+    :placeholder="placeholderText"
+    :aria-label="labelText"
+    @focus="$emit('focus')"
+  ></textarea>
 </template>

app/assets/javascripts/notes/components/note_form.vue

@@ -345,7 +345,7 @@ export default {
               class="note-textarea js-gfm-input js-note-text js-autosize markdown-area js-vue-issue-note-form"
               data-qa-selector="reply_field"
               dir="auto"
-              :aria-label="__('Description')"
+              :aria-label="__('Reply to comment')"
               :placeholder="__('Write a comment or drag your files here…')"
               @keydown.meta.enter="handleKeySubmit()"
               @keydown.ctrl.enter="handleKeySubmit()"

app/assets/stylesheets/framework/buttons.scss

@@ -216,6 +216,7 @@
     color: $gray-700;
   }
 
+  // deprecated class
   &.btn-text-field {
     width: 100%;
     text-align: left;

app/assets/stylesheets/framework/filters.scss

@@ -453,7 +453,7 @@
 .search-token-target-branch {
   .value {
     font-family: $monospace-font;
-    font-size: 13px;
+    font-size: $gl-font-size-monospace;
   }
 }
 

app/assets/stylesheets/framework/typography.scss

@@ -604,7 +604,7 @@ pre {
   display: block;
   padding: $gl-padding-8 $input-horizontal-padding;
   margin: 0 0 $gl-padding-8;
-  font-size: 13px;
+  font-size: $gl-font-size-monospace;
   word-break: break-all;
   word-wrap: break-word;
   color: $gl-text-color;
@@ -646,7 +646,7 @@ code {
  */
 textarea.js-gfm-input {
   font-family: $monospace-font;
-  font-size: 13px;
+  font-size: $gl-font-size-monospace;
 }
 
 .strikethrough {

app/assets/stylesheets/pages/note_form.scss

@@ -302,8 +302,20 @@ table {
       width: 100%;
     }
   }
+}
+
+.discussion-reply-holder {
+  .reply-placeholder-text-field {
+    font-family: $monospace-font;
+    font-size: $gl-font-size-monospace;
+    border-radius: $gl-border-radius-base;
+    width: 100%;
+    resize: none;
+    padding: $gl-padding-8 $gl-padding-12;
+    line-height: 1;
+    border-color: $border-color;
+    background-color: $white;
 
-  .btn-text-field {
     @include media-breakpoint-down(xs) {
       margin-bottom: $gl-padding-8;
     }

changelogs/unreleased/241058-mg-update-reply-placeholder.yml

+---
+title: Update accessibility of the "Reply to discussion" UX
+merge_request: 54380
+author:
+type: other

locale/gitlab.pot

@@ -18644,9 +18644,6 @@ msgstr ""
 msgid "MergeRequestDiffs|Select comment starting line"
 msgstr ""
 
-msgid "MergeRequests|Add a reply"
-msgstr ""
-
 msgid "MergeRequests|An error occurred while checking whether another squash is in progress."
 msgstr ""
 
@@ -18656,9 +18653,6 @@ msgstr ""
 msgid "MergeRequests|Failed to squash. Should be done manually."
 msgstr ""
 
-msgid "MergeRequests|Reply..."
-msgstr ""
-
 msgid "MergeRequests|Resolve this thread in a new issue"
 msgstr ""
 
@@ -19935,6 +19929,9 @@ msgstr ""
 msgid "New directory"
 msgstr ""
 
+msgid "New discussion"
+msgstr ""
+
 msgid "New environment"
 msgstr ""
 
@@ -24867,7 +24864,7 @@ msgstr ""
 msgid "Reply to this email directly or %{view_it_on_gitlab}."
 msgstr ""
 
-msgid "Reply..."
+msgid "Reply…"
 msgstr ""
 
 msgid "Repo by URL"
@@ -27974,7 +27971,7 @@ msgstr ""
 msgid "Start a Free Ultimate Trial"
 msgstr ""
 
-msgid "Start a new discussion..."
+msgid "Start a new discussion…"
 msgstr ""
 
 msgid "Start a new merge request"

spec/features/issues/gfm_autocomplete_spec.rb

@@ -994,7 +994,7 @@ RSpec.describe 'GFM autocomplete', :js do
   end
 
   def start_and_cancel_discussion
-    click_button('Reply...')
+    find_field('Reply…').click
 
     fill_in('note_note', with: 'Whoops!')
 

spec/features/merge_request/batch_comments_spec.rb

@@ -223,7 +223,7 @@ end
 
 def write_reply_to_discussion(button_text: 'Start a review', text: 'Line is wrong', resolve: false, unresolve: false)
   page.within(first('.diff-files-holder .discussion-reply-holder')) do
-    click_button('Reply...')
+    find_field('Reply…', match: :first).click
 
     fill_in('note_note', with: text)
 

spec/features/merge_request/user_posts_diff_notes_spec.rb

@@ -192,7 +192,7 @@ RSpec.describe 'Merge request > User posts diff notes', :js do
       it 'adds as discussion' do
         should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'), asset_form_reset: false)
         expect(page).to have_css('.notes_holder .note.note-discussion', count: 1)
-        expect(page).to have_button('Reply...')
+        expect(page).to have_field('Reply…')
       end
     end
   end

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -149,7 +149,7 @@ RSpec.describe 'Merge request > User resolves diff notes and threads', :js do
 
           it 'allows user to comment' do
             page.within '.diff-content' do
-              click_button 'Reply...'
+              find_field('Reply…').click
 
               find(".js-unresolve-checkbox").set false
               find('.js-note-text').set 'testing'
@@ -179,7 +179,7 @@ RSpec.describe 'Merge request > User resolves diff notes and threads', :js do
 
           it 'allows user to comment & unresolve thread' do
             page.within '.diff-content' do
-              click_button 'Reply...'
+              find_field('Reply…').click
 
               find('.js-note-text').set 'testing'
 
@@ -208,7 +208,7 @@ RSpec.describe 'Merge request > User resolves diff notes and threads', :js do
 
       it 'allows user to comment & resolve thread' do
         page.within '.diff-content' do
-          click_button 'Reply...'
+          find_field('Reply…').click
 
           find('.js-note-text').set 'testing'
 
@@ -442,7 +442,7 @@ RSpec.describe 'Merge request > User resolves diff notes and threads', :js do
 
       it 'allows user to comment & resolve thread' do
         page.within '.diff-content' do
-          click_button 'Reply...'
+          find_field('Reply…').click
 
           find('.js-note-text').set 'testing'
 
@@ -461,7 +461,7 @@ RSpec.describe 'Merge request > User resolves diff notes and threads', :js do
         page.within '.diff-content' do
           click_button 'Resolve thread'
 
-          click_button 'Reply...'
+          find_field('Reply…').click
 
           find('.js-note-text').set 'testing'
 

spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb

@@ -37,7 +37,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do
     end
 
     it 'does not render avatars after commenting on discussion tab' do
-      click_button 'Reply...'
+      find_field('Reply…').click
 
       page.within('.js-discussion-note-form') do
         find('.note-textarea').native.send_keys('Test comment')
@@ -132,7 +132,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do
       end
 
       it 'adds avatar when commenting' do
-        click_button 'Reply...'
+        find_field('Reply…', match: :first).click
 
         page.within '.js-discussion-note-form' do
           find('.js-note-text').native.send_keys('Test')
@@ -151,7 +151,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do
 
       it 'adds multiple comments' do
         3.times do
-          click_button 'Reply...'
+          find_field('Reply…', match: :first).click
 
           page.within '.js-discussion-note-form' do
             find('.js-note-text').native.send_keys('Test')

spec/features/merge_request/user_sees_discussions_spec.rb

@@ -60,7 +60,7 @@ RSpec.describe 'Merge request > User sees threads', :js do
 
       it 'can be replied to' do
         within(".discussion[data-discussion-id='#{discussion_id}']") do
-          click_button 'Reply...'
+          find_field('Reply…').click
           fill_in 'note[note]', with: 'Test!'
           click_button 'Comment'
 

spec/features/merge_request/user_sees_notes_from_forked_project_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe 'Merge request > User sees notes from forked project', :js do
     expect(page).to have_content('A commit comment')
 
     page.within('.discussion-notes') do
-      find('.btn-text-field').click
+      find_field('Reply…').click
       scroll_to(page.find('#note_note', visible: false))
       find('#note_note').send_keys('A reply comment')
       find('.js-comment-button').click

spec/features/merge_request/user_suggests_changes_on_diff_spec.rb

@@ -83,7 +83,7 @@ RSpec.describe 'User comments on a diff', :js do
 
       wait_for_requests
 
-      click_button 'Reply...'
+      find_field('Reply…', match: :first).click
 
       find('.js-suggestion-btn').click
 

spec/frontend/design_management/components/design_notes/design_discussion_spec.js

@@ -93,7 +93,7 @@ describe('Design discussions component', () => {
     });
 
     it('does not render a checkbox in reply form', () => {
-      findReplyPlaceholder().vm.$emit('onClick');
+      findReplyPlaceholder().vm.$emit('focus');
 
       return wrapper.vm.$nextTick().then(() => {
         expect(findResolveCheckbox().exists()).toBe(false);
@@ -124,7 +124,7 @@ describe('Design discussions component', () => {
     });
 
     it('renders a checkbox with Resolve thread text in reply form', () => {
-      findReplyPlaceholder().vm.$emit('onClick');
+      findReplyPlaceholder().vm.$emit('focus');
       wrapper.setProps({ discussionWithOpenForm: defaultMockDiscussion.id });
 
       return wrapper.vm.$nextTick().then(() => {
@@ -193,7 +193,7 @@ describe('Design discussions component', () => {
       });
 
       it('renders a checkbox with Unresolve thread text in reply form', () => {
-        findReplyPlaceholder().vm.$emit('onClick');
+        findReplyPlaceholder().vm.$emit('focus');
         wrapper.setProps({ discussionWithOpenForm: defaultMockDiscussion.id });
 
         return wrapper.vm.$nextTick().then(() => {
@@ -205,7 +205,7 @@ describe('Design discussions component', () => {
 
   it('hides reply placeholder and opens form on placeholder click', () => {
     createComponent();
-    findReplyPlaceholder().vm.$emit('onClick');
+    findReplyPlaceholder().vm.$emit('focus');
     wrapper.setProps({ discussionWithOpenForm: defaultMockDiscussion.id });
 
     return wrapper.vm.$nextTick().then(() => {
@@ -307,7 +307,7 @@ describe('Design discussions component', () => {
 
   it('emits openForm event on opening the form', () => {
     createComponent();
-    findReplyPlaceholder().vm.$emit('onClick');
+    findReplyPlaceholder().vm.$emit('focus');
 
     expect(wrapper.emitted('open-form')).toBeTruthy();
   });

spec/frontend/notes/components/discussion_actions_spec.js

@@ -96,7 +96,7 @@ describe('DiscussionActions', () => {
 
     it('emits showReplyForm event when clicking on reply placeholder', () => {
       jest.spyOn(wrapper.vm, '$emit');
-      wrapper.find(ReplyPlaceholder).find('button').trigger('click');
+      wrapper.find(ReplyPlaceholder).find('textarea').trigger('focus');
       expect(wrapper.vm.$emit).toHaveBeenCalledWith('showReplyForm');
     });
 

spec/frontend/notes/components/discussion_reply_placeholder_spec.js

 import { shallowMount } from '@vue/test-utils';
 import ReplyPlaceholder from '~/notes/components/discussion_reply_placeholder.vue';
 
-const buttonText = 'Test Button Text';
+const placeholderText = 'Test Button Text';
 
 describe('ReplyPlaceholder', () => {
   let wrapper;
 
-  const findButton = () => wrapper.find({ ref: 'button' });
+  const findTextarea = () => wrapper.find({ ref: 'textarea' });
 
   beforeEach(() => {
     wrapper = shallowMount(ReplyPlaceholder, {
       propsData: {
-        buttonText,
+        placeholderText,
       },
     });
   });
@@ -20,17 +20,17 @@ describe('ReplyPlaceholder', () => {
     wrapper.destroy();
   });
 
-  it('emits onClick event on button click', () => {
-    findButton().trigger('click');
+  it('emits focus event on button click', () => {
+    findTextarea().trigger('focus');
 
     return wrapper.vm.$nextTick().then(() => {
       expect(wrapper.emitted()).toEqual({
-        onClick: [[]],
+        focus: [[]],
       });
     });
   });
 
   it('should render reply button', () => {
-    expect(findButton().text()).toEqual(buttonText);
+    expect(findTextarea().attributes('placeholder')).toEqual(placeholderText);
   });
 });

spec/frontend/notes/components/noteable_discussion_spec.js

@@ -65,7 +65,7 @@ describe('noteable_discussion component', () => {
       expect(wrapper.vm.isReplying).toEqual(false);
 
       const replyPlaceholder = wrapper.find(ReplyPlaceholder);
-      replyPlaceholder.vm.$emit('onClick');
+      replyPlaceholder.vm.$emit('focus');
       await nextTick();
 
       expect(wrapper.vm.isReplying).toEqual(true);