From 40fa6d532fc778ab26546ec0764bad9cf15a3152 Mon Sep 17 00:00:00 2001
From: zarend <zach@ruist.com>
Date: Thu, 17 Jan 2019 21:53:12 -0800
Subject: [PATCH] fix(Dropdown): prevent calling onChange unless value changed

For the Dropdown component, ensures that onChange is called only if the user selectes a value that is different from the current value of the Dropdown. This is so we can have parity with the regular HTML select.
---
 src/modules/Dropdown/Dropdown.js             | 31 ++++++++++++--------
 test/specs/modules/Dropdown/Dropdown-test.js | 25 +++++++++++++---
 2 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/src/modules/Dropdown/Dropdown.js b/src/modules/Dropdown/Dropdown.js
index d5667e74c2..4bbc4ae42e 100644
--- a/src/modules/Dropdown/Dropdown.js
+++ b/src/modules/Dropdown/Dropdown.js
@@ -576,27 +576,32 @@ export default class Dropdown extends Component {
   }
 
   makeSelectedItemActive = (e) => {
-    const { open } = this.state
+    const { open, value } = this.state
     const { multiple } = this.props
 
     const item = this.getSelectedItem()
-    const value = _.get(item, 'value')
+    const selectedValue = _.get(item, 'value')
 
     // prevent selecting null if there was no selected item value
     // prevent selecting duplicate items when the dropdown is closed
-    if (_.isNil(value) || !open) return
+    if (_.isNil(selectedValue) || !open) return
 
     // state value may be undefined
-    const newValue = multiple ? _.union(this.state.value, [value]) : value
-
-    // notify the onChange prop that the user is trying to change value
-    this.setValue(newValue)
-    this.setSelectedIndex(newValue)
-    this.handleChange(e, newValue)
-
-    // Heads up! This event handler should be called after `onChange`
-    // Notify the onAddItem prop if this is a new value
-    if (item['data-additional']) _.invoke(this.props, 'onAddItem', e, { ...this.props, value })
+    const newValue = multiple ? _.union(this.state.value, [selectedValue]) : selectedValue
+    const valueHasChanged = multiple ? !!_.difference(newValue, value).length : newValue !== value
+
+    if (valueHasChanged) {
+      // notify the onChange prop that the user is trying to change value
+      this.setValue(newValue)
+      this.setSelectedIndex(newValue)
+      this.handleChange(e, newValue)
+
+      // Heads up! This event handler should be called after `onChange`
+      // Notify the onAddItem prop if this is a new value
+      if (item['data-additional']) {
+        _.invoke(this.props, 'onAddItem', e, { ...this.props, value: selectedValue })
+      }
+    }
   }
 
   selectItemOnEnter = (e) => {
diff --git a/test/specs/modules/Dropdown/Dropdown-test.js b/test/specs/modules/Dropdown/Dropdown-test.js
index 78c766cdbb..90f649db63 100644
--- a/test/specs/modules/Dropdown/Dropdown-test.js
+++ b/test/specs/modules/Dropdown/Dropdown-test.js
@@ -430,15 +430,32 @@ describe('Dropdown', () => {
       spy.should.have.been.calledWithMatch(event)
     })
 
-    it('calls makeSelectedItemActive', () => {
-      wrapperShallow(<Dropdown selectOnBlur />)
+    it('calls handleChange with the selected option on blur', () => {
+      wrapperShallow(<Dropdown selectOnBlur options={options} />)
 
       const instance = wrapper.instance()
-      sandbox.spy(instance, 'makeSelectedItemActive')
+      wrapper.simulate('click', { stopPropagation: _.noop })
+      dropdownMenuIsOpen()
+      sandbox.spy(instance, 'handleChange')
+
+      const event = { stopPropagation: _.noop }
+      wrapper.simulate('blur', event)
+
+      instance.handleChange.should.have.been.calledWithMatch(event, options[0].value)
+    })
+
+    it('does not call handleChange if the value has not changed', () => {
+      wrapperShallow(<Dropdown selectOnBlur options={options} />)
+
+      const instance = wrapper.instance()
+      wrapper.setState({ selectedIndex: 2, value: options[2].value })
+      wrapper.simulate('click', { stopPropagation: _.noop })
+      dropdownMenuIsOpen()
+      sandbox.spy(instance, 'handleChange')
 
       wrapper.simulate('blur')
 
-      instance.makeSelectedItemActive.should.have.been.calledOnce()
+      instance.handleChange.should.not.have.been.called()
     })
 
     it('sets focus state to false', () => {