Resolve "start discussion toggle clicking divider causes UI break"

app/assets/javascripts/droplab/constants.js

@@ -2,10 +2,12 @@ const DATA_TRIGGER = 'data-dropdown-trigger';
 const DATA_DROPDOWN = 'data-dropdown';
 const SELECTED_CLASS = 'droplab-item-selected';
 const ACTIVE_CLASS = 'droplab-item-active';
+const IGNORE_CLASS = 'droplab-item-ignore';
 
 export {
   DATA_TRIGGER,
   DATA_DROPDOWN,
   SELECTED_CLASS,
   ACTIVE_CLASS,
+  IGNORE_CLASS,
 };

app/assets/javascripts/droplab/drop_down.js

@@ -1,7 +1,7 @@
 /* eslint-disable */
 
 import utils from './utils';
-import { SELECTED_CLASS } from './constants';
+import { SELECTED_CLASS, IGNORE_CLASS } from './constants';
 
 var DropDown = function(list) {
   this.currentIndex = 0;
@@ -36,6 +36,7 @@ Object.assign(DropDown.prototype, {
 
   clickEvent: function(e) {
     if (e.target.tagName === 'UL') return;
+    if (e.target.classList.contains(IGNORE_CLASS)) return;
 
     var selected = utils.closest(e.target, 'LI');
     if (!selected) return;

app/assets/stylesheets/framework/dropdowns.scss

@@ -564,3 +564,7 @@
     color: $gl-text-color-secondary;
   }
 }
+
+.droplab-item-ignore {
+  pointer-events: none;
+}

app/views/projects/notes/_comment_button.html.haml

@@ -16,7 +16,7 @@
             %p
               Add a general comment to this #{noteable_name}.
 
-      %li.divider
+      %li.divider.droplab-item-ignore
 
       %li#discussion{ data: { value: 'DiscussionNote', 'submit-text' => 'Start discussion', 'close-text' => "Start discussion & close #{noteable_name}", 'reopen-text' => "Start discussion & reopen #{noteable_name}" } }
         %a{ href: '#' }

spec/javascripts/droplab/constants_spec.js

@@ -26,4 +26,10 @@ describe('constants', function () {
       expect(constants.ACTIVE_CLASS).toBe('droplab-item-active');
     });
   });
+
+  describe('IGNORE_CLASS', function () {
+    it('should be `droplab-item-ignore`', function() {
+      expect(constants.IGNORE_CLASS).toBe('droplab-item-ignore');
+    });
+  });
 });

spec/javascripts/droplab/drop_down_spec.js

@@ -2,7 +2,7 @@
 
 import DropDown from '~/droplab/drop_down';
 import utils from '~/droplab/utils';
-import { SELECTED_CLASS } from '~/droplab/constants';
+import { SELECTED_CLASS, IGNORE_CLASS } from '~/droplab/constants';
 
 describe('DropDown', function () {
   describe('class constructor', function () {
@@ -128,9 +128,10 @@ describe('DropDown', function () {
 
   describe('clickEvent', function () {
     beforeEach(function () {
+      this.classList = jasmine.createSpyObj('classList', ['contains']);
       this.list = { dispatchEvent: () => {} };
       this.dropdown = { hide: () => {}, list: this.list, addSelectedClass: () => {} };
-      this.event = { preventDefault: () => {}, target: {} };
+      this.event = { preventDefault: () => {}, target: { classList: this.classList } };
       this.customEvent = {};
       this.closestElement = {};
 
@@ -140,6 +141,7 @@ describe('DropDown', function () {
       spyOn(this.event, 'preventDefault');
       spyOn(window, 'CustomEvent').and.returnValue(this.customEvent);
       spyOn(utils, 'closest').and.returnValues(this.closestElement, undefined);
+      this.classList.contains.and.returnValue(false);
 
       DropDown.prototype.clickEvent.call(this.dropdown, this.event);
     });
@@ -164,13 +166,17 @@ describe('DropDown', function () {
       expect(window.CustomEvent).toHaveBeenCalledWith('click.dl', jasmine.any(Object));
     });
 
+    it('should call .classList.contains checking for IGNORE_CLASS', function () {
+      expect(this.classList.contains).toHaveBeenCalledWith(IGNORE_CLASS);
+    });
+
     it('should call .dispatchEvent with the customEvent', function () {
       expect(this.list.dispatchEvent).toHaveBeenCalledWith(this.customEvent);
     });
 
     describe('if the target is a UL element', function () {
       beforeEach(function () {
-        this.event = { preventDefault: () => {}, target: { tagName: 'UL' } };
+        this.event = { preventDefault: () => {}, target: { tagName: 'UL', classList: this.classList } };
 
         spyOn(this.event, 'preventDefault');
         utils.closest.calls.reset();
@@ -183,6 +189,22 @@ describe('DropDown', function () {
       });
     });
 
+    describe('if the target has the IGNORE_CLASS class', function () {
+      beforeEach(function () {
+        this.event = { preventDefault: () => {}, target: { tagName: 'LI', classList: this.classList } };
+
+        spyOn(this.event, 'preventDefault');
+        this.classList.contains.and.returnValue(true);
+        utils.closest.calls.reset();
+
+        DropDown.prototype.clickEvent.call(this.dropdown, this.event);
+      });
+
+      it('should return immediately', function () {
+        expect(utils.closest).not.toHaveBeenCalled();
+      });
+    });
+
     describe('if no selected element exists', function () {
       beforeEach(function () {
         this.event.preventDefault.calls.reset();

spec/support/features/discussion_comments_shared_example.rb

@@ -73,9 +73,15 @@ shared_examples 'discussion comments' do |resource_name|
       expect(page).not_to have_selector menu_selector
     end
 
-    it 'clicking the ul padding should not change the text' do
+    it 'clicking the ul padding or divider should not change the text' do
       find(menu_selector).trigger 'click'
 
+      expect(page).to have_selector menu_selector
+      expect(find(dropdown_selector)).to have_content 'Comment'
+
+      find("#{menu_selector} .divider").trigger 'click'
+
+      expect(page).to have_selector menu_selector
       expect(find(dropdown_selector)).to have_content 'Comment'
     end