fix(Slider): onRelease not always firing (#2695) (#5359)

* fix(Slider): onRelease not always firing (#2695) * test(windows): fixing failing unit tests * Updating jest `testMatch` micromatch expressions (jestjs/jest#7914) * Replacing `/` with `path.sep` in icon-build-helpers search.js * Replacing regex using `/` with `[\\/]` in test-utils scss.js * refactor(Slider): reworking event logic - Adding lodash.throttle as project dependency for use in Slider - Reworking Slider component's event handling - Including throttling in Slider event handling - Adjusting CSS for input element on Slider * test(Slider): updating/adding unit tests - Increasing (line) test coverage to 100% - Slight refactor of functions to be more testable - Correctly handling hidden text input - Correctly handling touchmove events - Gracefully handling edge case of zero-width bounding rect * fix(Slider): code review updates - Removing usage of functions that aren't available in all browsers - Adding unit test for display:none style on hidden text input * fix(Slider): code review updates - Removing throttling from keydown handling - Updating unit tests accordingly * refactor(Slider): use matches module Using `matches(...)` for arrow key event matching instead of custom impl Co-authored-by: Josh Black <[email protected]> Co-authored-by: TJ Egan <[email protected]> Co-authored-by: Abbey Hart <[email protected]>
carbon-design-system · Mar 10, 2020 · f5b7d0b · f5b7d0b
1 parent 2e18f24
commit f5b7d0b
Show file tree

Hide file tree

Showing 7 changed files with 482 additions and 222 deletions.
diff --git a/package.json b/package.json
@@ -175,8 +175,8 @@
     ],
     "testMatch": [
       "<rootDir>/**/__tests__/**/*.js?(x)",
-      "<rootDir>/**/?(*.)(spec|test).js?(x)",
-      "<rootDir>/**/?(*-)(spec|test).js?(x)"
+      "<rootDir>/**/*.(spec|test).js?(x)",
+      "<rootDir>/**/*-(spec|test).js?(x)"
     ],
     "transform": {
       "^.+\\.(js|jsx)$": "./tasks/jest/jsTransform.js",

diff --git a/packages/components/src/components/slider/_slider.scss b/packages/components/src/components/slider/_slider.scss
@@ -117,7 +117,6 @@
   .#{$prefix}-slider-text-input {
     width: rem(64px);
     height: rem(40px);
-    padding: 0;
     text-align: center;
     -moz-appearance: textfield;
 

diff --git a/packages/icon-build-helpers/src/search.js b/packages/icon-build-helpers/src/search.js
@@ -55,7 +55,7 @@ async function search(directory) {
     const dirname = path.dirname(filepath);
     const prefix = path
       .relative(directory, dirname)
-      .split('/')
+      .split(path.sep)
       .filter(Boolean);
     return {
       ...file,

diff --git a/packages/react/package.json b/packages/react/package.json
@@ -47,6 +47,7 @@
     "lodash.findlast": "^4.5.0",
     "lodash.isequal": "^4.5.0",
     "lodash.omit": "^4.5.0",
+    "lodash.throttle": "^4.1.1",
     "react-is": "^16.8.6",
     "warning": "^3.0.0",
     "window-or-global": "^1.0.1"

diff --git a/packages/react/src/components/Slider/Slider-test.js b/packages/react/src/components/Slider/Slider-test.js
@@ -15,9 +15,10 @@ import { settings } from 'carbon-components';
 const { prefix } = settings;
 describe('Slider', () => {
   describe('Renders as expected', () => {
+    const id = 'slider';
     const wrapper = mount(
       <Slider
-        id="slider"
+        id={id}
         className="extra-class"
         value={50}
         min={0}
@@ -55,6 +56,20 @@ describe('Slider', () => {
       wrapper.setProps({ light: true });
       expect(wrapper.props().light).toEqual(true);
     });
+
+    it('marks input field as hidden if hidden via props', () => {
+      wrapper.setProps({ hideTextInput: true });
+      expect(wrapper.find(`#${id}-input-for-slider`).props().type).toEqual(
+        'hidden'
+      );
+    });
+
+    it('sets style to display:none on input field if hidden via props', () => {
+      wrapper.setProps({ hideTextInput: true });
+      expect(wrapper.find(`#${id}-input-for-slider`).props().style).toEqual({
+        display: 'none',
+      });
+    });
   });
 
   describe('Supporting label', () => {
@@ -102,7 +117,7 @@ describe('Slider', () => {
     });
   });
 
-  describe('updatePosition method', () => {
+  describe('key/mouse event processing', () => {
     const handleChange = jest.fn();
     const handleRelease = jest.fn();
     const wrapper = mount(
@@ -113,6 +128,7 @@ describe('Slider', () => {
         min={0}
         max={100}
         step={1}
+        stepMultiplier={5}
         onChange={handleChange}
         onRelease={handleRelease}
       />
@@ -123,38 +139,78 @@ describe('Slider', () => {
         type: 'keydown',
         which: '38',
       };
-      wrapper.instance().updatePosition(evt);
-      expect(handleChange).lastCalledWith({ value: 51 });
+      wrapper.instance().onKeyDown(evt);
       expect(wrapper.state().value).toEqual(51);
+      expect(handleChange).lastCalledWith({ value: 51 });
     });
 
     it('sets correct state from event with a left/down keydown', () => {
       const evt = {
         type: 'keydown',
         which: '40',
       };
-      wrapper.instance().updatePosition(evt);
-      expect(handleChange).lastCalledWith({ value: 50 });
+      wrapper.instance().onKeyDown(evt);
       expect(wrapper.state().value).toEqual(50);
+      expect(handleChange).lastCalledWith({ value: 50 });
     });
 
-    it('sets correct state from event with a clientX', () => {
+    it('correctly uses setMultiplier with a right/up keydown', () => {
       const evt = {
-        type: 'click',
+        type: 'keydown',
+        which: '38',
+        shiftKey: true,
+      };
+      wrapper.instance().onKeyDown(evt);
+      expect(wrapper.state().value).toEqual(55);
+      expect(handleChange).lastCalledWith({ value: 55 });
+    });
+
+    it('sets correct state from event with a clientX in a mousemove', () => {
+      const evt = {
+        type: 'mousemove',
         clientX: '1000',
       };
-      wrapper.instance().updatePosition(evt);
+      wrapper.instance()._onDrag(evt);
       expect(handleChange).lastCalledWith({ value: 100 });
       expect(wrapper.state().value).toEqual(100);
     });
 
+    it('sets correct state from event with a clientX in a touchmove', () => {
+      const evt = {
+        type: 'touchmove',
+        touches: [{ clientX: '0' }],
+      };
+      wrapper.instance()._onDrag(evt);
+      expect(handleChange).lastCalledWith({ value: 0 });
+      expect(wrapper.state().value).toEqual(0);
+    });
+
+    it('throttles mousemove events', () => {
+      const evt1 = {
+        type: 'mousemove',
+        clientX: '1000',
+      };
+      const evt2 = {
+        type: 'mousemove',
+        clientX: '0',
+      };
+      wrapper.instance().onDrag(evt1);
+      wrapper.instance().onDrag(evt2);
+      expect(wrapper.state().value).toEqual(100);
+      expect(handleChange).lastCalledWith({ value: 100 });
+    });
+
     describe('user is holding the handle', () => {
       it('does not call onRelease', () => {
+        const evt = {
+          type: 'mousemove',
+          clientX: '1000',
+        };
         handleRelease.mockClear();
         expect(handleRelease).not.toHaveBeenCalled();
 
-        wrapper.instance().handleMouseStart();
-        wrapper.instance().updatePosition();
+        wrapper.instance().onDragStart(evt);
+        wrapper.instance().onDrag(evt);
         expect(handleRelease).not.toHaveBeenCalled();
       });
     });
@@ -164,12 +220,151 @@ describe('Slider', () => {
         handleRelease.mockClear();
         expect(handleRelease).not.toHaveBeenCalled();
         wrapper.setState({
-          holding: false,
+          needsOnRelease: true,
         });
-        wrapper.instance().updatePosition();
+        wrapper.instance().onDragStop();
         expect(handleRelease).toHaveBeenCalled();
       });
     });
+
+    it('sets correct state when typing in input field', () => {
+      const evt = {
+        target: {
+          value: '999',
+        },
+      };
+      wrapper.instance().onChange(evt);
+      expect(wrapper.state().value).toEqual(100);
+      expect(handleChange).lastCalledWith({ value: 100 });
+    });
+  });
+
+  describe('error handling', () => {
+    const handleChange = jest.fn();
+    const handleRelease = jest.fn();
+    const wrapper = mount(
+      <Slider
+        id="slider"
+        className="extra-class"
+        value={50}
+        min={0}
+        max={100}
+        step={1}
+        onChange={handleChange}
+        onRelease={handleRelease}
+      />
+    );
+
+    it('handles non-number typed into input field', () => {
+      const evt = {
+        target: {
+          value: '',
+        },
+      };
+      wrapper.instance().onChange(evt);
+      expect(wrapper.state().value).toEqual('');
+      expect(handleChange).not.toHaveBeenCalled();
+    });
+
+    it('gracefully tolerates empty event passed to _onDrag', () => {
+      const evt = {};
+      wrapper.instance()._onDrag(evt);
+      expect(wrapper.state().value).toEqual(''); // from last test
+      expect(handleChange).not.toHaveBeenCalled();
+    });
+
+    it('gracefully tolerates empty event passed to onChange', () => {
+      const evt = {};
+      wrapper.instance().onChange(evt);
+      expect(wrapper.state().value).toEqual(''); // from last test
+      expect(handleChange).not.toHaveBeenCalled();
+    });
+
+    it('gracefully tolerates empty event passed to onKeyDown', () => {
+      const evt = {};
+      wrapper.instance().onKeyDown(evt);
+      expect(wrapper.state().value).toEqual(''); // from last test
+      expect(handleChange).not.toHaveBeenCalled();
+    });
+
+    it('gracefully tolerates bad key code passed to onKeyDown', () => {
+      const evt = {
+        which: '123',
+      };
+      wrapper.instance().onKeyDown(evt);
+      expect(wrapper.state().value).toEqual(''); // from last test
+      expect(handleChange).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('slider is disabled', () => {
+    const handleChange = jest.fn();
+    const handleRelease = jest.fn();
+    const wrapper = mount(
+      <Slider
+        id="slider"
+        className="extra-class"
+        value={50}
+        min={0}
+        max={100}
+        step={1}
+        onChange={handleChange}
+        onRelease={handleRelease}
+        disabled={true}
+      />
+    );
+
+    it('does nothing when trying to type in the input', () => {
+      const evt = {
+        target: {
+          value: '',
+        },
+      };
+      wrapper.instance().onChange(evt);
+      expect(wrapper.state().value).toEqual(50);
+      expect(handleChange).not.toHaveBeenCalled();
+    });
+
+    it('does nothing when trying to start a drag', () => {
+      const evt = {
+        type: 'mousedown',
+        clientX: '1001',
+      };
+      wrapper.instance().onDragStart(evt);
+      expect(wrapper.state().value).toEqual(50);
+      expect(handleChange).not.toHaveBeenCalled();
+    });
+
+    it('does nothing when trying to drag', () => {
+      const evt = {
+        type: 'mousemove',
+        clientX: '1000',
+      };
+      wrapper.instance()._onDrag(evt);
+      expect(wrapper.state().value).toEqual(50);
+      expect(handleChange).not.toHaveBeenCalled();
+    });
+
+    it('does nothing when trying to stop a drag', () => {
+      const evt = {
+        type: 'mouseup',
+        clientX: '1001',
+      };
+      wrapper.instance().onDragStop(evt);
+      expect(wrapper.state().needsOnRelease).toEqual(false);
+      expect(handleChange).not.toHaveBeenCalled();
+      expect(handleRelease).not.toHaveBeenCalled();
+    });
+
+    it('does nothing when using arrow key', () => {
+      const evt = {
+        type: 'keydown',
+        which: '40',
+      };
+      wrapper.instance().onKeyDown(evt);
+      expect(wrapper.state().value).toEqual(50);
+      expect(handleChange).not.toHaveBeenCalled();
+    });
   });
 });