From a5498b401f7d43c103e4910c88f4dba71ce814e7 Mon Sep 17 00:00:00 2001 From: Joe Harvey Date: Fri, 14 Feb 2020 12:24:52 -0500 Subject: [PATCH 1/7] fix(Slider): onRelease not always firing (#2695) --- .../src/components/Slider/Slider-test.js | 8 +- .../react/src/components/Slider/Slider.js | 178 ++++++++++++------ 2 files changed, 131 insertions(+), 55 deletions(-) diff --git a/packages/react/src/components/Slider/Slider-test.js b/packages/react/src/components/Slider/Slider-test.js index c3974ce790eb..31763495617b 100644 --- a/packages/react/src/components/Slider/Slider-test.js +++ b/packages/react/src/components/Slider/Slider-test.js @@ -150,10 +150,15 @@ describe('Slider', () => { describe('user is holding the handle', () => { it('does not call onRelease', () => { + const evt = { + type: 'mousedown', + clientX: '1000', + persist: jest.fn(), + }; handleRelease.mockClear(); expect(handleRelease).not.toHaveBeenCalled(); - wrapper.instance().handleMouseStart(); + wrapper.instance().handleMouseStart(evt); wrapper.instance().updatePosition(); expect(handleRelease).not.toHaveBeenCalled(); }); @@ -165,6 +170,7 @@ describe('Slider', () => { expect(handleRelease).not.toHaveBeenCalled(); wrapper.setState({ holding: false, + needsOnRelease: true, }); wrapper.instance().updatePosition(); expect(handleRelease).toHaveBeenCalled(); diff --git a/packages/react/src/components/Slider/Slider.js b/packages/react/src/components/Slider/Slider.js index 3d62b5aa8af4..ad1e0f051baf 100644 --- a/packages/react/src/components/Slider/Slider.js +++ b/packages/react/src/components/Slider/Slider.js @@ -17,6 +17,38 @@ const defaultFormatLabel = (value, label) => { return typeof label === 'function' ? label(value) : `${value}${label}`; }; +/** + * Types of events used to indicate that the slider's value should get recalculated. + */ +const CALC_VALUE_EVENT_TYPES = Object.freeze([ + 'mousemove', + 'mousedown', + 'click', + 'touchmove', + 'touchstart', +]); + +/** + * Notes on Slider event handling: + * - The Slider handles 6 types of events. Five of them are defined in the `CALC_VALUE_EVENT_TYPES` + * array, while the last one, `mouseup`, is added on-the-fly in response to a `mousedown` event. + * 'mouseup' will remove itself as an event listener when fired. + * + * - All of the event handlers eventually drop into `this.updatePosition`. + * + * - `this.updatePosition` serves 3 main roles: 1) Calculating a new value and thumb position; 2) + * Updating the state with the new values; 3) Requesting an animation frame from the browser, + * during which the `onChange` and `onRelease` callbacks are potentially called. + * + * - `requestAnimationFrame` is used to rate-limit the number of events that are sent back to the + * component user as the slider changes. + * + * - As the value/thumb position are updated, `this.state.needsOnChange` and + * `this.state.needsOnRelease` may be set to true, indicating that during the next requested + * animation frame, the `onChange` and/or `onRelease` callbacks should be called. + * - Events fired with types other than those listed in `CALC_VALUE_EVENT_TYPES` will not result in + * the value/thumb position being recalculated. + */ export default class Slider extends PureComponent { static propTypes = { /** @@ -143,10 +175,11 @@ export default class Slider extends PureComponent { }; state = { - dragging: false, holding: false, value: this.props.value, left: 0, + needsOnChange: false, + needsOnRelease: false, }; static getDerivedStateFromProps({ value, min, max }, state) { @@ -176,41 +209,56 @@ export default class Slider extends PureComponent { evt.persist(); } - if (this.state.dragging) { - return; - } - this.setState({ dragging: true }); - - this.handleDrag(); - - requestAnimationFrame(() => { - this.setState((prevState, props) => { - // Note: In FF, `evt.target` of `mousemove` event can be `HTMLDocument` which doesn't have `classList`. - // One example is dragging out of browser viewport. - const fromInput = - evt && - evt.target && - evt.target.classList && - evt.target.classList.contains('bx-slider-text-input'); - const { left, newValue: newSliderValue } = this.calcValue( - evt, - prevState, - props - ); - const newValue = fromInput ? Number(evt.target.value) : newSliderValue; - if (prevState.left === left && prevState.value === newValue) { - return { dragging: false }; + const setStateUpdater = (prevState, props) => { + // Note: In FF, `evt.target` of `mousemove` event can be `HTMLDocument` which doesn't have + // `classList`. One example is dragging out of browser viewport. + const fromInput = + evt && + evt.target && + evt.target.classList && + evt.target.classList.contains('bx-slider-text-input'); + + const { left, newValue: newSliderValue } = this.calcValue( + evt, + prevState, + props + ); + + const newValue = fromInput ? Number(evt.target.value) : newSliderValue; + + if (prevState.left === left && prevState.value === newValue) { + return; + } + + return { + left, + value: newValue, + needsOnChange: true, + }; + }; + + const setStateCallback = () => { + this.handleDragComplete(); + + requestAnimationFrame(() => { + if (this.state.needsOnChange) { + if (typeof this.props.onChange === 'function') { + this.props.onChange({ value: this.state.value }); + } } - if (typeof props.onChange === 'function') { - props.onChange({ value: newValue }); + if (this.state.needsOnRelease) { + if (typeof this.props.onRelease === 'function') { + this.props.onRelease({ value: this.state.value }); + } } - return { - dragging: false, - left, - value: newValue, - }; + this.setState({ + needsOnChange: false, + needsOnRelease: false, + }); }); - }); + }; + + this.setState(setStateUpdater, setStateCallback); }; calcValue = (evt, prevState, props) => { @@ -248,7 +296,8 @@ export default class Slider extends PureComponent { newValue = Number(value) + stepMultiplied * direction; } } - if (type === 'mousemove' || type === 'click' || type === 'touchmove') { + // If the event type indicates that we should update, then recalculate the value. + if (CALC_VALUE_EVENT_TYPES.includes(type)) { const clientX = evt.touches ? evt.touches[0].clientX : evt.clientX; const track = this.track.getBoundingClientRect(); const ratio = (clientX - track.left) / track.width; @@ -270,10 +319,38 @@ export default class Slider extends PureComponent { return { left, newValue }; }; - handleMouseStart = () => { - this.setState({ - holding: true, - }); + /** + * Check if dragging is done. If so, request an `onRelease` by setting `needsOnRelease` in the + * component state. + */ + handleDragComplete = () => { + if ( + typeof this.props.onRelease === 'function' && + !this.props.disabled && + !this.state.holding + ) { + this.setState({ + needsOnRelease: true, + }); + } + }; + + handleMouseStart = evt => { + if (!evt) { + return; + } else { + // Persist the synthetic event so it can be accessed below in setState + evt.persist(); + } + + this.setState( + { + holding: true, + }, + () => { + this.updatePosition(evt); + } + ); this.element.ownerDocument.addEventListener( 'mousemove', @@ -301,9 +378,12 @@ export default class Slider extends PureComponent { }; handleTouchStart = () => { - this.setState({ - holding: true, - }); + this.setState( + { + holding: true, + }, + this.updatePosition + ); this.element.ownerDocument.addEventListener( 'touchmove', this.updatePosition @@ -350,16 +430,6 @@ export default class Slider extends PureComponent { this.updatePosition(evt); }; - handleDrag = () => { - if ( - typeof this.props.onRelease === 'function' && - !this.props.disabled && - !this.state.holding - ) { - this.props.onRelease({ value: this.state.value }); - } - }; - render() { const { ariaLabelInput, @@ -433,6 +503,9 @@ export default class Slider extends PureComponent { }} onClick={this.updatePosition} onKeyPress={this.updatePosition} + onMouseDown={this.handleMouseStart} + onTouchStart={this.handleTouchStart} + onKeyDown={this.updatePosition} role="presentation" tabIndex={-1} {...other}> @@ -445,9 +518,6 @@ export default class Slider extends PureComponent { aria-valuemin={min} aria-valuenow={value} style={thumbStyle} - onMouseDown={this.handleMouseStart} - onTouchStart={this.handleTouchStart} - onKeyDown={this.updatePosition} />
Date: Fri, 14 Feb 2020 12:32:59 -0500 Subject: [PATCH 2/7] test(windows): fixing failing unit tests * Updating jest `testMatch` micromatch expressions (facebook/jest#7914) * Replacing `/` with `path.sep` in icon-build-helpers search.js * Replacing regex using `/` with `[\\/]` in test-utils scss.js --- package.json | 4 ++-- packages/icon-build-helpers/src/search.js | 2 +- packages/test-utils/scss.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index 72f9d7a9c5af..25ad8d5a358a 100644 --- a/package.json +++ b/package.json @@ -175,8 +175,8 @@ ], "testMatch": [ "/**/__tests__/**/*.js?(x)", - "/**/?(*.)(spec|test).js?(x)", - "/**/?(*-)(spec|test).js?(x)" + "/**/*.(spec|test).js?(x)", + "/**/*-(spec|test).js?(x)" ], "transform": { "^.+\\.(js|jsx)$": "./tasks/jest/jsTransform.js", diff --git a/packages/icon-build-helpers/src/search.js b/packages/icon-build-helpers/src/search.js index 07c71bfcb7dd..4412d0d40d5c 100644 --- 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/test-utils/scss.js b/packages/test-utils/scss.js index 878f3a4079d4..8866615f1fa7 100644 --- a/packages/test-utils/scss.js +++ b/packages/test-utils/scss.js @@ -51,7 +51,7 @@ function createImporter(cwd) { }, pathFilter(pkg, path, relativePath) { // Transforms `scss/filename` to `scss/_filename.scss` - return relativePath.replace(/^(scss\/)([a-z-]+)/, '$1_$2.scss'); + return relativePath.replace(/^(scss[\\/])([a-z-]+)/, '$1_$2.scss'); }, }); done({ file }); From 2545e7a5f82e1d3e8607f68c23aa379eef72a075 Mon Sep 17 00:00:00 2001 From: Joe Harvey Date: Mon, 24 Feb 2020 14:05:42 -0500 Subject: [PATCH 3/7] 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 --- .../src/components/slider/_slider.scss | 1 - packages/react/package.json | 1 + .../react/src/components/Slider/Slider.js | 498 +++++++++--------- yarn.lock | 22 +- 4 files changed, 255 insertions(+), 267 deletions(-) diff --git a/packages/components/src/components/slider/_slider.scss b/packages/components/src/components/slider/_slider.scss index b1bad6ed745c..f42f85af31b1 100644 --- 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/react/package.json b/packages/react/package.json index 66ca2f4be944..5812b7bd126a 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -47,6 +47,7 @@ "lodash.debounce": "^4.0.8", "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.js b/packages/react/src/components/Slider/Slider.js index ad1e0f051baf..a71f825e462f 100644 --- a/packages/react/src/components/Slider/Slider.js +++ b/packages/react/src/components/Slider/Slider.js @@ -9,6 +9,8 @@ import React, { PureComponent } from 'react'; import PropTypes from 'prop-types'; import classNames from 'classnames'; import { settings } from 'carbon-components'; +import throttle from 'lodash.throttle'; + import deprecate from '../../prop-types/deprecate'; const { prefix } = settings; @@ -18,37 +20,37 @@ const defaultFormatLabel = (value, label) => { }; /** - * Types of events used to indicate that the slider's value should get recalculated. + * Minimum time between processed "drag" events. */ -const CALC_VALUE_EVENT_TYPES = Object.freeze([ - 'mousemove', - 'mousedown', - 'click', - 'touchmove', - 'touchstart', -]); +const EVENT_THROTTLE = 16; // ms /** - * Notes on Slider event handling: - * - The Slider handles 6 types of events. Five of them are defined in the `CALC_VALUE_EVENT_TYPES` - * array, while the last one, `mouseup`, is added on-the-fly in response to a `mousedown` event. - * 'mouseup' will remove itself as an event listener when fired. - * - * - All of the event handlers eventually drop into `this.updatePosition`. - * - * - `this.updatePosition` serves 3 main roles: 1) Calculating a new value and thumb position; 2) - * Updating the state with the new values; 3) Requesting an animation frame from the browser, - * during which the `onChange` and `onRelease` callbacks are potentially called. - * - * - `requestAnimationFrame` is used to rate-limit the number of events that are sent back to the - * component user as the slider changes. - * - * - As the value/thumb position are updated, `this.state.needsOnChange` and - * `this.state.needsOnRelease` may be set to true, indicating that during the next requested - * animation frame, the `onChange` and/or `onRelease` callbacks should be called. - * - Events fired with types other than those listed in `CALC_VALUE_EVENT_TYPES` will not result in - * the value/thumb position being recalculated. + * Event types that trigger "drags". + */ +const DRAG_EVENT_TYPES = new Set(['mousemove', 'touchmove']); + +/** + * Event types that trigger a "drag" to stop. */ +const DRAG_STOP_EVENT_TYPES = new Set(['mouseup', 'touchend', 'touchcancel']); + +/** + * Constants used during keydown event processing. + */ +const ARROW_KEYS = Object.freeze({ + ArrowLeft: 37, + ArrowUp: 38, + ArrowRight: 39, + ArrowDown: 40, +}); + +// Define a Math.clamp function if the browser doesn't have it (ECMA2017+) +if (!('clamp' in Math)) { + Math.clamp = function(val, min, max) { + return Math.max(min, Math.min(val, max)); + }; +} + export default class Slider extends PureComponent { static propTypes = { /** @@ -175,259 +177,255 @@ export default class Slider extends PureComponent { }; state = { - holding: false, value: this.props.value, left: 0, - needsOnChange: false, needsOnRelease: false, }; - static getDerivedStateFromProps({ value, min, max }, state) { - const { value: currentValue, prevValue, prevMin, prevMax } = state; - if (prevValue === value && prevMin === min && prevMax === max) { - return null; - } - const effectiveValue = Math.min( - Math.max(prevValue === value ? currentValue : value, min), - max - ); - return { - value: effectiveValue, - left: ((effectiveValue - min) / (max - min)) * 100, - prevValue: value, - prevMin: min, - prevMax: max, - }; + /** + * Sets up initial slider position and value in response to component mount. + */ + componentDidMount() { + const { value, left } = this.calcValue({}); + this.setState({ value, left }); } - updatePosition = evt => { - if (evt && this.props.disabled) { - return; + /** + * Handles firing of `onChange` and `onRelease` callbacks to parent in + * response to state changes. + * + * @param {*} _ Unused (prevProps) + * @param {*} prevState The previous Slider state, used to see if callbacks + * should be called. + */ + componentDidUpdate(_, prevState) { + // Fire onChange event handler, if present, if there's a usable value, and + // if the value is different from the last one + if ( + this.state.value !== '' && + prevState.value !== this.state.value && + typeof this.props.onChange === 'function' + ) { + this.props.onChange({ value: this.state.value }); } - if (evt && evt.dispatchConfig) { - evt.persist(); + // Fire onRelease event handler, if present and if needed + if ( + this.state.needsOnRelease && + typeof this.props.onRelease === 'function' + ) { + this.props.onRelease({ value: this.state.value }); + // Reset the flag + this.setState({ needsOnRelease: false }); } + } - const setStateUpdater = (prevState, props) => { - // Note: In FF, `evt.target` of `mousemove` event can be `HTMLDocument` which doesn't have - // `classList`. One example is dragging out of browser viewport. - const fromInput = - evt && - evt.target && - evt.target.classList && - evt.target.classList.contains('bx-slider-text-input'); - - const { left, newValue: newSliderValue } = this.calcValue( - evt, - prevState, - props - ); - - const newValue = fromInput ? Number(evt.target.value) : newSliderValue; - - if (prevState.left === left && prevState.value === newValue) { - return; - } + /** + * Sets up "drag" event handlers and calls `this.onDrag` in case dragging + * started on somewhere other than the thumb without a corresponding "move" + * event. + * + * @param {Event} evt The event. + */ + onDragStart = evt => { + // Do nothing if component is disabled + if (this.props.disabled) { + return; + } - return { - left, - value: newValue, - needsOnChange: true, - }; - }; + // Register drag stop handlers + DRAG_STOP_EVENT_TYPES.forEach(element => { + this.element.ownerDocument.addEventListener(element, this.onDragStop); + }); - const setStateCallback = () => { - this.handleDragComplete(); - - requestAnimationFrame(() => { - if (this.state.needsOnChange) { - if (typeof this.props.onChange === 'function') { - this.props.onChange({ value: this.state.value }); - } - } - if (this.state.needsOnRelease) { - if (typeof this.props.onRelease === 'function') { - this.props.onRelease({ value: this.state.value }); - } - } - this.setState({ - needsOnChange: false, - needsOnRelease: false, - }); - }); - }; + // Register drag handlers + DRAG_EVENT_TYPES.forEach(element => { + this.element.ownerDocument.addEventListener(element, this.onDrag); + }); - this.setState(setStateUpdater, setStateCallback); + // Perform first recalculation since we probably didn't click exactly in the + // middle of the thumb + this.onDrag(evt); }; - calcValue = (evt, prevState, props) => { - const { min, max, step, stepMuliplier, stepMultiplier } = props; + /** + * Unregisters "drag" and "drag stop" event handlers and calls sets the flag + * inidicating that the `onRelease` callback should be called. + */ + onDragStop = () => { + // Do nothing if component is disabled + if (this.props.disabled) { + return; + } - const { value } = prevState; + // Remove drag stop handlers + DRAG_STOP_EVENT_TYPES.forEach(element => { + this.element.ownerDocument.removeEventListener(element, this.onDragStop); + }); - const range = max - min; - const valuePercentage = ((value - min) / range) * 100; + // Remove drag handlers + DRAG_EVENT_TYPES.forEach(element => { + this.element.ownerDocument.removeEventListener(element, this.onDrag); + }); - let left; - let newValue; - left = valuePercentage; - newValue = value; + // Set needsOnRelease flag so event fires on next update + this.setState({ needsOnRelease: true }); + }; - if (evt) { - const { type } = evt; + /** + * Handles a "drag" event by recalculating the value/thumb and setting state + * accordingly. This function throttles "drag" events to be processed at most + * once every `EVENT_THROTTLE` milliseconds. + * + * @param {Event} evt The event. + */ + onDrag = throttle( + evt => { + // Do nothing if component is disabled + if (this.props.disabled) { + return; + } - if (type === 'keydown') { - const direction = { - 40: -1, // decreasing - 37: -1, // decreasing - 38: 1, // increasing - 39: 1, // increasing - }[evt.which]; + // Do nothing if we have no valid event or clientX + if (!evt || !('clientX' in evt)) { + return; + } - const multiplyStep = stepMuliplier || stepMultiplier; + const { value, left } = this.calcValue({ clientX: evt.clientX }); + this.setState({ value, left }); + }, + EVENT_THROTTLE, + { leading: true, trailing: false } + ); - if (direction !== undefined) { - const multiplier = - evt.shiftKey === true ? range / step / multiplyStep : 1; - const stepMultiplied = step * multiplier; - const stepSize = (stepMultiplied / range) * 100; - left = valuePercentage + stepSize * direction; - newValue = Number(value) + stepMultiplied * direction; - } + /** + * Handles a `keydown` event by recalculating the value/thumb and setting + * state accordingly. This function throttles "drag" events to be processed at + * most once every `EVENT_THROTTLE` milliseconds. + * + * @param {Event} evt The event. + */ + onKeyDown = throttle( + evt => { + // Do nothing if component is disabled + if (this.props.disabled) { + return; } - // If the event type indicates that we should update, then recalculate the value. - if (CALC_VALUE_EVENT_TYPES.includes(type)) { - const clientX = evt.touches ? evt.touches[0].clientX : evt.clientX; - const track = this.track.getBoundingClientRect(); - const ratio = (clientX - track.left) / track.width; - const rounded = min + Math.round((range * ratio) / step) * step; - left = ((rounded - min) / range) * 100; - newValue = rounded; + + // Make sure it's a key we want to handle + if (!('which' in evt) || !Object.values(ARROW_KEYS).includes(evt.which)) { + return; } - } - if (newValue <= Number(min)) { - left = 0; - newValue = min; - } - if (newValue >= Number(max)) { - left = 100; - newValue = max; - } + let delta = 0; + switch (evt.which) { + case ARROW_KEYS.ArrowDown: + case ARROW_KEYS.ArrowLeft: + delta = -this.props.step; + break; + case ARROW_KEYS.ArrowUp: + case ARROW_KEYS.ArrowRight: + delta = this.props.step; + break; + default: + return; + } - return { left, newValue }; - }; + // If shift was held, account for the stepMultiplier + if (evt.shiftKey) { + const stepMultiplier = + this.props.stepMultiplier || this.props.stepMuliplier; + delta *= stepMultiplier; + } + + const { value, left } = this.calcValue({ + value: this.state.value + delta, + }); + this.setState({ value, left }); + }, + EVENT_THROTTLE, + { leading: true, trailing: false } + ); /** - * Check if dragging is done. If so, request an `onRelease` by setting `needsOnRelease` in the - * component state. + * Provides the two-way binding for the input field of the Slider. It also + * Handles a change to the input field by recalculating the value/thumb and + * setting state accordingly. + * + * @param {Event} evt The event. */ - handleDragComplete = () => { - if ( - typeof this.props.onRelease === 'function' && - !this.props.disabled && - !this.state.holding - ) { - this.setState({ - needsOnRelease: true, - }); + onChange = evt => { + // Do nothing if component is disabled + if (this.props.disabled) { + return; } - }; - handleMouseStart = evt => { - if (!evt) { + // Do nothing if we have no valid event, target, or value + if (!evt || !('target' in evt) || typeof evt.target.value !== 'string') { return; - } else { - // Persist the synthetic event so it can be accessed below in setState - evt.persist(); } - this.setState( - { - holding: true, - }, - () => { - this.updatePosition(evt); - } - ); + let targetValue = Number.parseFloat(evt.target.value); - this.element.ownerDocument.addEventListener( - 'mousemove', - this.updatePosition - ); - this.element.ownerDocument.addEventListener('mouseup', this.handleMouseEnd); - }; - - handleMouseEnd = () => { - this.setState( - { - holding: false, - }, - this.updatePosition - ); + // Avoid calling calcValue for invaid numbers, but still update the state + if (Number.isNaN(targetValue)) { + this.setState({ value: evt.target.value }); + } else { + targetValue = Math.clamp(targetValue, this.props.min, this.props.max); - this.element.ownerDocument.removeEventListener( - 'mousemove', - this.updatePosition - ); - this.element.ownerDocument.removeEventListener( - 'mouseup', - this.handleMouseEnd - ); + // Recalculate the state's value and update the Slider + const { value, left } = this.calcValue({ value: targetValue }); + this.setState({ value, left, needsOnRelease: true }); + } }; - handleTouchStart = () => { - this.setState( - { - holding: true, - }, - this.updatePosition - ); - this.element.ownerDocument.addEventListener( - 'touchmove', - this.updatePosition - ); - this.element.ownerDocument.addEventListener('touchup', this.handleTouchEnd); - this.element.ownerDocument.addEventListener( - 'touchend', - this.handleTouchEnd - ); - this.element.ownerDocument.addEventListener( - 'touchcancel', - this.handleTouchEnd - ); - }; + /** + * Calculates a new Slider `value` and `left` (thumb offset) given a `clientX`, + * `value`, or neither of those. + * - If `clientX` is specified, it will be used in + * conjunction with the Slider's bounding rectangle to calculate the new + * values. + * - If `clientX` is not specified and `value` is, it will be used to + * calculate new values as though it were the current value of the Slider. + * - If neither `clientX` nor `value` are specified, `this.props.value` will + * be used to calculate the new values as though it were the current value + * of the Slider. + * + * @param {object} params + * @param {number} [params.clientX] Optional clientX value expected to be from + * an event fired by one of the Slider's `DRAG_EVENT_TYPES` events. + * @param {number} [params.value] Optional value use during calculations if + * clientX is not provided. + */ + calcValue = ({ clientX = null, value = null }) => { + const range = this.props.max - this.props.min; + const boundingRect = this.element.getBoundingClientRect(); + const width = boundingRect.right - boundingRect.left; + const totalSteps = range / this.props.step; + + // If a clientX is specified, use it to calculate the leftPercent. If not, + // use the provided value or state's value to calculate it instead. + let leftPercent; + if (clientX != null) { + const leftOffset = clientX - boundingRect.left; + leftPercent = leftOffset / width; + } else { + if (value == null) { + value = this.state.value; + } + leftPercent = value / (range - this.props.min); + } - handleTouchEnd = () => { - this.setState( - { - holding: false, - }, - this.updatePosition - ); + let steppedValue = Math.round(leftPercent * totalSteps) * this.props.step; + let steppedPercent = Math.clamp(steppedValue / range, 0, 1); - this.element.ownerDocument.removeEventListener( - 'touchmove', - this.updatePosition - ); - this.element.ownerDocument.removeEventListener( - 'touchup', - this.handleTouchEnd - ); - this.element.ownerDocument.removeEventListener( - 'touchend', - this.handleTouchEnd - ); - this.element.ownerDocument.removeEventListener( - 'touchcancel', - this.handleTouchEnd + steppedValue = Math.clamp( + steppedValue + this.props.min, + this.props.min, + this.props.max ); - }; - handleChange = evt => { - this.setState({ value: evt.target.value }); - this.updatePosition(evt); + return { value: steppedValue, left: steppedPercent * 100 }; }; render() { @@ -501,11 +499,9 @@ export default class Slider extends PureComponent { ref={node => { this.element = node; }} - onClick={this.updatePosition} - onKeyPress={this.updatePosition} - onMouseDown={this.handleMouseStart} - onTouchStart={this.handleTouchStart} - onKeyDown={this.updatePosition} + onMouseDown={this.onDragStart} + onTouchStart={this.onDragStart} + onKeyDown={this.onKeyDown} role="presentation" tabIndex={-1} {...other}> @@ -529,17 +525,6 @@ export default class Slider extends PureComponent { className={`${prefix}--slider__filled-track`} style={filledTrackStyle} /> -
{formatLabel(max, maxLabel)} @@ -548,11 +533,16 @@ export default class Slider extends PureComponent { )} diff --git a/yarn.lock b/yarn.lock index 5dba2fc33c13..0dcfeab3f6fa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -18410,7 +18410,7 @@ react-docgen@^5.0.0: node-dir "^0.1.10" strip-indent "^3.0.0" -react-dom@^16.6.0, react-dom@^16.8.3, react-dom@^16.8.6, react-dom@^16.9.0, react-dom@~16.9.0: +react-dom@^16.6.0, react-dom@^16.8.3, react-dom@^16.8.6, react-dom@^16.9.0: version "16.9.0" resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-16.9.0.tgz#5e65527a5e26f22ae3701131bcccaee9fb0d3962" integrity sha512-YFT2rxO9hM70ewk9jq0y6sQk8cL02xm4+IzYBz75CQGlClQQ1Bxq0nhHF6OtSbit+AIahujJgb/CPRibFkMNJQ== @@ -18498,7 +18498,12 @@ react-inspector@^4.0.0: prop-types "^15.6.1" storybook-chromatic "^2.2.2" -react-is@^16.12.0, react-is@^16.7.0, react-is@^16.8.1, react-is@^16.8.2, react-is@^16.8.3, react-is@^16.8.4, react-is@^16.8.6, react-is@^16.9.0, react-is@~16.9.0: +react-is@^16.12.0: + version "16.12.0" + resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.12.0.tgz#2cc0fe0fba742d97fd527c42a13bec4eeb06241c" + integrity sha512-rPCkf/mWBtKc97aLL9/txD8DZdemK0vkA3JMLShjlJB3Pj3s+lpf1KaBzMfQrAmhMQB0n1cU/SUGgKKBCe837Q== + +react-is@^16.7.0, react-is@^16.8.1, react-is@^16.8.2, react-is@^16.8.3, react-is@^16.8.4, react-is@^16.8.6, react-is@^16.9.0: version "16.9.0" resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.9.0.tgz#21ca9561399aad0ff1a7701c01683e8ca981edcb" integrity sha512-tJBzzzIgnnRfEm046qRcURvwQnZVXmuCbscxUO5RWrGTXpon2d4c8mI0D8WE6ydVIm29JiLB6+RslkIvym9Rjw== @@ -18576,7 +18581,7 @@ react-syntax-highlighter@^11.0.2: prismjs "^1.8.4" refractor "^2.4.1" -react-test-renderer@^16.0.0-0, react-test-renderer@^16.8.6, react-test-renderer@~16.9.0: +react-test-renderer@^16.0.0-0, react-test-renderer@^16.8.6: version "16.9.0" resolved "https://registry.yarnpkg.com/react-test-renderer/-/react-test-renderer-16.9.0.tgz#7ed657a374af47af88f66f33a3ef99c9610c8ae9" integrity sha512-R62stB73qZyhrJo7wmCW9jgl/07ai+YzvouvCXIJLBkRlRqLx4j9RqcLEAfNfU3OxTGucqR2Whmn3/Aad6L3hQ== @@ -18604,7 +18609,7 @@ react-transition-group@^2.2.1: prop-types "^15.6.2" react-lifecycles-compat "^3.0.4" -react@^16.6.0, react@^16.8.3, react@^16.8.6, react@^16.9.0, react@~16.9.0: +react@^16.6.0, react@^16.8.3, react@^16.8.6, react@^16.9.0: version "16.9.0" resolved "https://registry.yarnpkg.com/react/-/react-16.9.0.tgz#40ba2f9af13bc1a38d75dbf2f4359a5185c4f7aa" integrity sha512-+7LQnFBwkiw+BobzOF6N//BdoNw0ouwmSJTEm9cglOOmsg/TMiFHZLe2sEoN5M7LgJTj9oHH0gxklfnQe66S1w== @@ -19716,14 +19721,7 @@ rollup-pluginutils@2.0.1: estree-walker "^0.3.0" micromatch "^2.3.11" -rollup-pluginutils@2.8.2: - version "2.8.2" - resolved "https://registry.yarnpkg.com/rollup-pluginutils/-/rollup-pluginutils-2.8.2.tgz#72f2af0748b592364dbd3389e600e5a9444a351e" - integrity sha512-EEp9NhnUkwY8aif6bxgovPHMoMoNr2FulJziTndpt5H9RdwC47GSGuII9XxpSdzVGM0GWrNPHV6ie1LTNJPaLQ== - dependencies: - estree-walker "^0.6.1" - -rollup-pluginutils@^2.6.0, rollup-pluginutils@^2.8.1: +rollup-pluginutils@2.8.2, rollup-pluginutils@^2.6.0, rollup-pluginutils@^2.8.1: version "2.8.2" resolved "https://registry.yarnpkg.com/rollup-pluginutils/-/rollup-pluginutils-2.8.2.tgz#72f2af0748b592364dbd3389e600e5a9444a351e" integrity sha512-EEp9NhnUkwY8aif6bxgovPHMoMoNr2FulJziTndpt5H9RdwC47GSGuII9XxpSdzVGM0GWrNPHV6ie1LTNJPaLQ== From dd3f739f3fd23403bfbb07d2be978a560f5aeff3 Mon Sep 17 00:00:00 2001 From: Joe Harvey Date: Tue, 25 Feb 2020 01:48:33 -0500 Subject: [PATCH 4/7] 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 --- .../src/components/Slider/Slider-test.js | 223 ++++++++++++++++-- .../react/src/components/Slider/Slider.js | 171 ++++++++------ yarn.lock | 13 +- 3 files changed, 308 insertions(+), 99 deletions(-) diff --git a/packages/react/src/components/Slider/Slider-test.js b/packages/react/src/components/Slider/Slider-test.js index 31763495617b..1805f512ff9d 100644 --- 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( { wrapper.setProps({ light: true }); expect(wrapper.props().light).toEqual(true); }); + + it('marks the input field as hidden if specified in props', () => { + wrapper.setProps({ hideTextInput: true }); + expect(wrapper.find(`#${id}-input-for-slider`).props().type).toEqual( + 'hidden' + ); + }); }); describe('Supporting label', () => { @@ -102,7 +110,7 @@ describe('Slider', () => { }); }); - describe('updatePosition method', () => { + describe('key/mouse event processing', () => { const handleChange = jest.fn(); const handleRelease = jest.fn(); const wrapper = mount( @@ -113,6 +121,7 @@ describe('Slider', () => { min={0} max={100} step={1} + stepMultiplier={5} onChange={handleChange} onRelease={handleRelease} /> @@ -123,9 +132,9 @@ 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', () => { @@ -133,33 +142,79 @@ describe('Slider', () => { 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('correctly uses setMultiplier with a right/up keydown', () => { + const evt = { + 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', () => { + it('sets correct state from event with a clientX in a mousemove', () => { const evt = { - type: 'click', + 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 keydown events', () => { + const evt = { + type: 'keydown', + which: '38', + }; + wrapper.instance().onKeyDown(evt); + wrapper.instance().onKeyDown(evt); + expect(wrapper.state().value).toEqual(1); + expect(handleChange).lastCalledWith({ value: 1 }); + }); + + 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: 'mousedown', + type: 'mousemove', clientX: '1000', - persist: jest.fn(), }; handleRelease.mockClear(); expect(handleRelease).not.toHaveBeenCalled(); - wrapper.instance().handleMouseStart(evt); - wrapper.instance().updatePosition(); + wrapper.instance().onDragStart(evt); + wrapper.instance().onDrag(evt); expect(handleRelease).not.toHaveBeenCalled(); }); }); @@ -169,13 +224,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( + + ); + + 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( + + ); + + 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(); + }); }); }); diff --git a/packages/react/src/components/Slider/Slider.js b/packages/react/src/components/Slider/Slider.js index a71f825e462f..6bf6a87d229a 100644 --- a/packages/react/src/components/Slider/Slider.js +++ b/packages/react/src/components/Slider/Slider.js @@ -199,7 +199,7 @@ export default class Slider extends PureComponent { * should be called. */ componentDidUpdate(_, prevState) { - // Fire onChange event handler, if present, if there's a usable value, and + // Fire onChange event handler if present, if there's a usable value, and // if the value is different from the last one if ( this.state.value !== '' && @@ -209,7 +209,7 @@ export default class Slider extends PureComponent { this.props.onChange({ value: this.state.value }); } - // Fire onRelease event handler, if present and if needed + // Fire onRelease event handler if present and if needed if ( this.state.needsOnRelease && typeof this.props.onRelease === 'function' @@ -274,78 +274,92 @@ export default class Slider extends PureComponent { /** * Handles a "drag" event by recalculating the value/thumb and setting state - * accordingly. This function throttles "drag" events to be processed at most - * once every `EVENT_THROTTLE` milliseconds. + * accordingly. * * @param {Event} evt The event. */ - onDrag = throttle( - evt => { - // Do nothing if component is disabled - if (this.props.disabled) { - return; - } + _onDrag = evt => { + // Do nothing if component is disabled or we have no event + if (this.props.disabled || !evt) { + return; + } - // Do nothing if we have no valid event or clientX - if (!evt || !('clientX' in evt)) { - return; - } + let clientX; + if ('clientX' in evt) { + clientX = evt.clientX; + } else if ( + 'touches' in evt && + 0 in evt.touches && + 'clientX' in evt.touches[0] + ) { + clientX = evt.touches[0].clientX; + } else { + // Do nothing if we have no valid clientX + return; + } - const { value, left } = this.calcValue({ clientX: evt.clientX }); - this.setState({ value, left }); - }, - EVENT_THROTTLE, - { leading: true, trailing: false } - ); + const { value, left } = this.calcValue({ clientX }); + this.setState({ value, left }); + }; /** - * Handles a `keydown` event by recalculating the value/thumb and setting - * state accordingly. This function throttles "drag" events to be processed at + * Throttles calls to `this._onDrag` by limiting events to being processed at * most once every `EVENT_THROTTLE` milliseconds. + */ + onDrag = throttle(this._onDrag, EVENT_THROTTLE, { + leading: true, + trailing: false, + }); + + /** + * Handles a `keydown` event by recalculating the value/thumb and setting + * state accordingly. * * @param {Event} evt The event. */ - onKeyDown = throttle( - evt => { - // Do nothing if component is disabled - if (this.props.disabled) { - return; - } + _onKeyDown = evt => { + // Do nothing if component is disabled or we don't have a valid event + if (this.props.disabled || !('which' in evt)) { + return; + } - // Make sure it's a key we want to handle - if (!('which' in evt) || !Object.values(ARROW_KEYS).includes(evt.which)) { + const which = Number.parseInt(evt.which); + let delta = 0; + switch (which) { + case ARROW_KEYS.ArrowDown: + case ARROW_KEYS.ArrowLeft: + delta = -this.props.step; + break; + case ARROW_KEYS.ArrowUp: + case ARROW_KEYS.ArrowRight: + delta = this.props.step; + break; + default: + // Ignore keys we don't want to handle return; - } + } - let delta = 0; - switch (evt.which) { - case ARROW_KEYS.ArrowDown: - case ARROW_KEYS.ArrowLeft: - delta = -this.props.step; - break; - case ARROW_KEYS.ArrowUp: - case ARROW_KEYS.ArrowRight: - delta = this.props.step; - break; - default: - return; - } + // If shift was held, account for the stepMultiplier + if (evt.shiftKey) { + const stepMultiplier = + this.props.stepMultiplier || this.props.stepMuliplier; + delta *= stepMultiplier; + } - // If shift was held, account for the stepMultiplier - if (evt.shiftKey) { - const stepMultiplier = - this.props.stepMultiplier || this.props.stepMuliplier; - delta *= stepMultiplier; - } + const { value, left } = this.calcValue({ + value: this.state.value + delta, + }); + this.setState({ value, left }); + }; - const { value, left } = this.calcValue({ - value: this.state.value + delta, - }); - this.setState({ value, left }); - }, - EVENT_THROTTLE, - { leading: true, trailing: false } - ); + /** + * Throttles calls to `this._onKeyDown` by limiting events to being processed + * at most once every `EVENT_THROTTLE` milliseconds. + */ + onKeyDown = throttle(this._onKeyDown, EVENT_THROTTLE, { + leading: true, + trailing: false, + }); /** * Provides the two-way binding for the input field of the Slider. It also @@ -400,8 +414,13 @@ export default class Slider extends PureComponent { calcValue = ({ clientX = null, value = null }) => { const range = this.props.max - this.props.min; const boundingRect = this.element.getBoundingClientRect(); - const width = boundingRect.right - boundingRect.left; const totalSteps = range / this.props.step; + let width = boundingRect.right - boundingRect.left; + + // Enforce a minimum width of at least 1 for calculations + if (width <= 0) { + width = 1; + } // If a clientX is specified, use it to calculate the leftPercent. If not, // use the provided value or state's value to calculate it instead. @@ -484,6 +503,9 @@ export default class Slider extends PureComponent { const thumbStyle = { left: `${left}%`, }; + const hiddenInputStyle = { + display: 'none', + }; return (
@@ -529,22 +551,21 @@ export default class Slider extends PureComponent { {formatLabel(max, maxLabel)} - {!hideTextInput && ( - - )} +
); diff --git a/yarn.lock b/yarn.lock index a6f8c5bd3eaa..e560b5eee5cc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -18415,7 +18415,7 @@ react-docgen@^5.0.0: node-dir "^0.1.10" strip-indent "^3.0.0" -react-dom@^16.6.0, react-dom@^16.8.3, react-dom@^16.8.6, react-dom@^16.9.0: +react-dom@^16.6.0, react-dom@^16.8.3, react-dom@^16.8.6, react-dom@^16.9.0, react-dom@~16.9.0: version "16.9.0" resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-16.9.0.tgz#5e65527a5e26f22ae3701131bcccaee9fb0d3962" integrity sha512-YFT2rxO9hM70ewk9jq0y6sQk8cL02xm4+IzYBz75CQGlClQQ1Bxq0nhHF6OtSbit+AIahujJgb/CPRibFkMNJQ== @@ -18503,12 +18503,7 @@ react-inspector@^4.0.0: prop-types "^15.6.1" storybook-chromatic "^2.2.2" -react-is@^16.12.0: - version "16.12.0" - resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.12.0.tgz#2cc0fe0fba742d97fd527c42a13bec4eeb06241c" - integrity sha512-rPCkf/mWBtKc97aLL9/txD8DZdemK0vkA3JMLShjlJB3Pj3s+lpf1KaBzMfQrAmhMQB0n1cU/SUGgKKBCe837Q== - -react-is@^16.7.0, react-is@^16.8.1, react-is@^16.8.2, react-is@^16.8.3, react-is@^16.8.4, react-is@^16.8.6, react-is@^16.9.0: +react-is@^16.12.0, react-is@^16.7.0, react-is@^16.8.1, react-is@^16.8.2, react-is@^16.8.3, react-is@^16.8.4, react-is@^16.8.6, react-is@^16.9.0, react-is@~16.9.0: version "16.9.0" resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.9.0.tgz#21ca9561399aad0ff1a7701c01683e8ca981edcb" integrity sha512-tJBzzzIgnnRfEm046qRcURvwQnZVXmuCbscxUO5RWrGTXpon2d4c8mI0D8WE6ydVIm29JiLB6+RslkIvym9Rjw== @@ -18586,7 +18581,7 @@ react-syntax-highlighter@^11.0.2: prismjs "^1.8.4" refractor "^2.4.1" -react-test-renderer@^16.0.0-0, react-test-renderer@^16.8.6: +react-test-renderer@^16.0.0-0, react-test-renderer@^16.8.6, react-test-renderer@~16.9.0: version "16.9.0" resolved "https://registry.yarnpkg.com/react-test-renderer/-/react-test-renderer-16.9.0.tgz#7ed657a374af47af88f66f33a3ef99c9610c8ae9" integrity sha512-R62stB73qZyhrJo7wmCW9jgl/07ai+YzvouvCXIJLBkRlRqLx4j9RqcLEAfNfU3OxTGucqR2Whmn3/Aad6L3hQ== @@ -18614,7 +18609,7 @@ react-transition-group@^2.2.1: prop-types "^15.6.2" react-lifecycles-compat "^3.0.4" -react@^16.6.0, react@^16.8.3, react@^16.8.6, react@^16.9.0: +react@^16.6.0, react@^16.8.3, react@^16.8.6, react@^16.9.0, react@~16.9.0: version "16.9.0" resolved "https://registry.yarnpkg.com/react/-/react-16.9.0.tgz#40ba2f9af13bc1a38d75dbf2f4359a5185c4f7aa" integrity sha512-+7LQnFBwkiw+BobzOF6N//BdoNw0ouwmSJTEm9cglOOmsg/TMiFHZLe2sEoN5M7LgJTj9oHH0gxklfnQe66S1w== From 57827fdd4611200dde20998149734048986966f1 Mon Sep 17 00:00:00 2001 From: "Joseph D. Harvey" Date: Tue, 25 Feb 2020 11:16:51 -0500 Subject: [PATCH 5/7] 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 --- .../src/components/Slider/Slider-test.js | 9 +++++- .../react/src/components/Slider/Slider.js | 28 +++++++++++-------- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/packages/react/src/components/Slider/Slider-test.js b/packages/react/src/components/Slider/Slider-test.js index 1805f512ff9d..2350a36b72a8 100644 --- a/packages/react/src/components/Slider/Slider-test.js +++ b/packages/react/src/components/Slider/Slider-test.js @@ -57,12 +57,19 @@ describe('Slider', () => { expect(wrapper.props().light).toEqual(true); }); - it('marks the input field as hidden if specified in props', () => { + 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', () => { diff --git a/packages/react/src/components/Slider/Slider.js b/packages/react/src/components/Slider/Slider.js index 6bf6a87d229a..fb57cf5146d8 100644 --- a/packages/react/src/components/Slider/Slider.js +++ b/packages/react/src/components/Slider/Slider.js @@ -44,13 +44,6 @@ const ARROW_KEYS = Object.freeze({ ArrowDown: 40, }); -// Define a Math.clamp function if the browser doesn't have it (ECMA2017+) -if (!('clamp' in Math)) { - Math.clamp = function(val, min, max) { - return Math.max(min, Math.min(val, max)); - }; -} - export default class Slider extends PureComponent { static propTypes = { /** @@ -220,6 +213,19 @@ export default class Slider extends PureComponent { } } + /** + * Synonymous to ECMA2017+ `Math.clamp`. + * + * @param {number} val + * @param {number} min + * @param {number} max + * + * @returns `val` if `max>=val>=min`; `min` if `valmax`. + */ + clamp(val, min, max) { + return Math.max(min, Math.min(val, max)); + } + /** * Sets up "drag" event handlers and calls `this.onDrag` in case dragging * started on somewhere other than the thumb without a corresponding "move" @@ -382,10 +388,10 @@ export default class Slider extends PureComponent { let targetValue = Number.parseFloat(evt.target.value); // Avoid calling calcValue for invaid numbers, but still update the state - if (Number.isNaN(targetValue)) { + if (isNaN(targetValue)) { this.setState({ value: evt.target.value }); } else { - targetValue = Math.clamp(targetValue, this.props.min, this.props.max); + targetValue = this.clamp(targetValue, this.props.min, this.props.max); // Recalculate the state's value and update the Slider const { value, left } = this.calcValue({ value: targetValue }); @@ -436,9 +442,9 @@ export default class Slider extends PureComponent { } let steppedValue = Math.round(leftPercent * totalSteps) * this.props.step; - let steppedPercent = Math.clamp(steppedValue / range, 0, 1); + let steppedPercent = this.clamp(steppedValue / range, 0, 1); - steppedValue = Math.clamp( + steppedValue = this.clamp( steppedValue + this.props.min, this.props.min, this.props.max From c38670ed44071f5ea77cce2caef90a57ebbfa11e Mon Sep 17 00:00:00 2001 From: "Joseph D. Harvey" Date: Tue, 25 Feb 2020 11:26:13 -0500 Subject: [PATCH 6/7] fix(Slider): code review updates - Removing throttling from keydown handling - Updating unit tests accordingly --- .../src/components/Slider/Slider-test.js | 27 ++++++------------- .../react/src/components/Slider/Slider.js | 11 +------- 2 files changed, 9 insertions(+), 29 deletions(-) diff --git a/packages/react/src/components/Slider/Slider-test.js b/packages/react/src/components/Slider/Slider-test.js index 2350a36b72a8..e88def03f200 100644 --- a/packages/react/src/components/Slider/Slider-test.js +++ b/packages/react/src/components/Slider/Slider-test.js @@ -139,7 +139,7 @@ describe('Slider', () => { type: 'keydown', which: '38', }; - wrapper.instance()._onKeyDown(evt); + wrapper.instance().onKeyDown(evt); expect(wrapper.state().value).toEqual(51); expect(handleChange).lastCalledWith({ value: 51 }); }); @@ -149,7 +149,7 @@ describe('Slider', () => { type: 'keydown', which: '40', }; - wrapper.instance()._onKeyDown(evt); + wrapper.instance().onKeyDown(evt); expect(wrapper.state().value).toEqual(50); expect(handleChange).lastCalledWith({ value: 50 }); }); @@ -160,7 +160,7 @@ describe('Slider', () => { which: '38', shiftKey: true, }; - wrapper.instance()._onKeyDown(evt); + wrapper.instance().onKeyDown(evt); expect(wrapper.state().value).toEqual(55); expect(handleChange).lastCalledWith({ value: 55 }); }); @@ -185,17 +185,6 @@ describe('Slider', () => { expect(wrapper.state().value).toEqual(0); }); - it('throttles keydown events', () => { - const evt = { - type: 'keydown', - which: '38', - }; - wrapper.instance().onKeyDown(evt); - wrapper.instance().onKeyDown(evt); - expect(wrapper.state().value).toEqual(1); - expect(handleChange).lastCalledWith({ value: 1 }); - }); - it('throttles mousemove events', () => { const evt1 = { type: 'mousemove', @@ -291,18 +280,18 @@ describe('Slider', () => { expect(handleChange).not.toHaveBeenCalled(); }); - it('gracefully tolerates empty event passed to _onKeydown', () => { + it('gracefully tolerates empty event passed to onKeyDown', () => { const evt = {}; - wrapper.instance()._onKeyDown(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', () => { + it('gracefully tolerates bad key code passed to onKeyDown', () => { const evt = { which: '123', }; - wrapper.instance()._onKeyDown(evt); + wrapper.instance().onKeyDown(evt); expect(wrapper.state().value).toEqual(''); // from last test expect(handleChange).not.toHaveBeenCalled(); }); @@ -372,7 +361,7 @@ describe('Slider', () => { type: 'keydown', which: '40', }; - wrapper.instance()._onKeyDown(evt); + wrapper.instance().onKeyDown(evt); expect(wrapper.state().value).toEqual(50); expect(handleChange).not.toHaveBeenCalled(); }); diff --git a/packages/react/src/components/Slider/Slider.js b/packages/react/src/components/Slider/Slider.js index fb57cf5146d8..f173235bbe1a 100644 --- a/packages/react/src/components/Slider/Slider.js +++ b/packages/react/src/components/Slider/Slider.js @@ -323,7 +323,7 @@ export default class Slider extends PureComponent { * * @param {Event} evt The event. */ - _onKeyDown = evt => { + onKeyDown = evt => { // Do nothing if component is disabled or we don't have a valid event if (this.props.disabled || !('which' in evt)) { return; @@ -358,15 +358,6 @@ export default class Slider extends PureComponent { this.setState({ value, left }); }; - /** - * Throttles calls to `this._onKeyDown` by limiting events to being processed - * at most once every `EVENT_THROTTLE` milliseconds. - */ - onKeyDown = throttle(this._onKeyDown, EVENT_THROTTLE, { - leading: true, - trailing: false, - }); - /** * Provides the two-way binding for the input field of the Slider. It also * Handles a change to the input field by recalculating the value/thumb and From dcae2261ab0de5f3bee81b9b27286ccf35968f59 Mon Sep 17 00:00:00 2001 From: "Joseph D. Harvey" Date: Fri, 28 Feb 2020 17:33:56 -0500 Subject: [PATCH 7/7] refactor(Slider): use matches module Using `matches(...)` for arrow key event matching instead of custom impl --- .../react/src/components/Slider/Slider.js | 31 ++++++------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/packages/react/src/components/Slider/Slider.js b/packages/react/src/components/Slider/Slider.js index f173235bbe1a..c1c29fd1d573 100644 --- a/packages/react/src/components/Slider/Slider.js +++ b/packages/react/src/components/Slider/Slider.js @@ -11,6 +11,8 @@ import classNames from 'classnames'; import { settings } from 'carbon-components'; import throttle from 'lodash.throttle'; +import * as keys from '../../internal/keyboard/keys'; +import { matches } from '../../internal/keyboard/match'; import deprecate from '../../prop-types/deprecate'; const { prefix } = settings; @@ -34,16 +36,6 @@ const DRAG_EVENT_TYPES = new Set(['mousemove', 'touchmove']); */ const DRAG_STOP_EVENT_TYPES = new Set(['mouseup', 'touchend', 'touchcancel']); -/** - * Constants used during keydown event processing. - */ -const ARROW_KEYS = Object.freeze({ - ArrowLeft: 37, - ArrowUp: 38, - ArrowRight: 39, - ArrowDown: 40, -}); - export default class Slider extends PureComponent { static propTypes = { /** @@ -331,18 +323,13 @@ export default class Slider extends PureComponent { const which = Number.parseInt(evt.which); let delta = 0; - switch (which) { - case ARROW_KEYS.ArrowDown: - case ARROW_KEYS.ArrowLeft: - delta = -this.props.step; - break; - case ARROW_KEYS.ArrowUp: - case ARROW_KEYS.ArrowRight: - delta = this.props.step; - break; - default: - // Ignore keys we don't want to handle - return; + if (matches(which, [keys.ArrowDown, keys.ArrowLeft])) { + delta = -this.props.step; + } else if (matches(which, [keys.ArrowUp, keys.ArrowRight])) { + delta = this.props.step; + } else { + // Ignore keys we don't want to handle + return; } // If shift was held, account for the stepMultiplier