From 641d0eceef38ac7f367c8fefcc5fad3befc021db Mon Sep 17 00:00:00 2001 From: Josefina Mancilla <32556167+jnm2377@users.noreply.github.com> Date: Mon, 13 Sep 2021 15:00:35 -0500 Subject: [PATCH] fix(Slider): check validity of number input after onChange (#9132) * feat(slider): check input validity after changes * fix: slider invalid padding * fix: story example for testing * fix: invalid prop state sync * fix: update derivedStateFromProps * chore: update tests * fix: derivedStateFromProps return null * fix: set validity as true for drag and key evt * fix: update tests Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --- .../src/components/slider/_slider.scss | 4 + .../src/components/Slider/Slider-story.js | 2 +- .../src/components/Slider/Slider-test.js | 77 +++++++++++++++++-- .../react/src/components/Slider/Slider.js | 71 +++++++++++++---- .../scss/components/slider/_slider.scss | 4 + 5 files changed, 136 insertions(+), 22 deletions(-) diff --git a/packages/components/src/components/slider/_slider.scss b/packages/components/src/components/slider/_slider.scss index 82fbf92cf011..5315229e4717 100644 --- a/packages/components/src/components/slider/_slider.scss +++ b/packages/components/src/components/slider/_slider.scss @@ -128,6 +128,10 @@ } } + .#{$prefix}--slider-text-input.#{$prefix}--text-input--invalid { + padding-right: 1rem; + } + .#{$prefix}--slider__thumb:focus ~ .#{$prefix}--slider__filled-track { background-color: $interactive-04; } diff --git a/packages/react/src/components/Slider/Slider-story.js b/packages/react/src/components/Slider/Slider-story.js index 35766508d0f4..f2a34bc12a00 100644 --- a/packages/react/src/components/Slider/Slider-story.js +++ b/packages/react/src/components/Slider/Slider-story.js @@ -58,7 +58,7 @@ export const Default = () => ( { describe('key/mouse event processing', () => { const handleChange = jest.fn(); const handleRelease = jest.fn(); + const handleBlur = jest.fn(); + const wrapper = mount( { stepMultiplier={5} onChange={handleChange} onRelease={handleRelease} + onBlur={handleBlur} /> ); @@ -224,24 +227,36 @@ describe('Slider', () => { }); }); - it('does not set incorrect state when typing a invalid value in input field', () => { + it('allows user to set invalid value when typing in input field', () => { const evt = { target: { value: '999', - checkValidity: () => false, }, }; wrapper.instance().onChange(evt); - expect(wrapper.state().value).toEqual(100); - expect(handleChange).toHaveBeenLastCalledWith({ value: 100 }); + expect(wrapper.state().value).toEqual(999); + expect(handleChange).toHaveBeenLastCalledWith({ value: 999 }); + }); + + it('checks for validity onBlur', () => { + const checkValidity = jest.fn(); + + const evt = { + target: { + value: '', + checkValidity: checkValidity, + }, + }; + + wrapper.instance().onBlur(evt); + expect(checkValidity).toHaveBeenCalled(); }); it('sets correct state when typing a valid value in input field', () => { const evt = { target: { value: '12', - checkValidity: () => true, }, }; @@ -251,6 +266,51 @@ describe('Slider', () => { }); }); + describe('syncs invalid state and props', () => { + const handleChange = jest.fn(); + const handleRelease = jest.fn(); + const handleBlur = jest.fn(); + + const wrapper = mount( + + ); + + it('overrides invalid state for invalid prop', () => { + const changeEvt = { + target: { + value: '200', + }, + }; + + const blurEvt = { + target: { + checkValidity: () => false, + }, + }; + + wrapper.instance().onChange(changeEvt); + wrapper.instance().onBlur(blurEvt); + expect(wrapper.state().value).toEqual(200); + expect(wrapper.state().isValid).toEqual(true); + expect( + wrapper + .find(`.${prefix}--slider-text-input`) + .hasClass(`${prefix}--text-input--invalid`) + ).toEqual(false); + }); + }); + describe('error handling', () => { const handleChange = jest.fn(); const handleRelease = jest.fn(); @@ -292,6 +352,13 @@ describe('Slider', () => { expect(handleChange).not.toHaveBeenCalled(); }); + it('gracefully tolerates empty event passed to onBlur', () => { + const evt = {}; + wrapper.instance().onBlur(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); diff --git a/packages/react/src/components/Slider/Slider.js b/packages/react/src/components/Slider/Slider.js index 7d03e8ff311a..84ce82017968 100644 --- a/packages/react/src/components/Slider/Slider.js +++ b/packages/react/src/components/Slider/Slider.js @@ -178,6 +178,7 @@ export default class Slider extends PureComponent { value: this.props.value, left: 0, needsOnRelease: false, + isValid: true, }; /** @@ -302,7 +303,7 @@ export default class Slider extends PureComponent { }); // Set needsOnRelease flag so event fires on next update - this.setState({ needsOnRelease: true }); + this.setState({ needsOnRelease: true, isValid: true }); }; /** @@ -332,7 +333,7 @@ export default class Slider extends PureComponent { } const { value, left } = this.calcValue({ clientX }); - this.setState({ value, left }); + this.setState({ value, left, isValid: true }); }; /** @@ -383,7 +384,8 @@ export default class Slider extends PureComponent { ? Math.floor(this.state.value / this.props.step) * this.props.step : this.state.value) + delta, }); - this.setState({ value, left }); + + this.setState({ value, left, isValid: true }); }; /** @@ -393,6 +395,7 @@ export default class Slider extends PureComponent { * * @param {Event} evt The event. */ + onChange = (evt) => { // Do nothing if component is disabled if (this.props.disabled) { @@ -410,18 +413,34 @@ export default class Slider extends PureComponent { if (isNaN(targetValue)) { this.setState({ value: evt.target.value }); } else { - // Recalculate the state's value and update the Slider - // if it is a valid number - if (evt.target.checkValidity() === false) { - return; - } - const { value, left } = this.calcValue({ value: targetValue, useRawValue: true, }); - this.setState({ value, left, needsOnRelease: true }); + this.setState({ + value, + left, + needsOnRelease: true, + }); + } + }; + + /** + * Checks for validity of input value after clicking out of the input. It also + * Handles state change to isValid state. + * + * @param {Event} evt The event. + */ + onBlur = (evt) => { + // Do nothing if we have no valid event, target, or value + if (!evt || !('target' in evt) || typeof evt.target.value !== 'string') { + return; } + // determine validity of input change after clicking out of input + const validity = evt.target.checkValidity(); + this.setState({ + isValid: validity, + }); }; /** @@ -488,6 +507,25 @@ export default class Slider extends PureComponent { return { value: steppedValue, left: steppedPercent * 100 }; }; + // syncs invalid state and prop + static getDerivedStateFromProps(props, state) { + const { isValid } = state; + // will override state in favor of invalid prop + if (props.invalid === true && isValid === true) { + return { + isValid: false, + }; + } + + if (props.invalid === false && isValid === false) { + return { + isValid: true, + }; + } + //if invalid prop is not provided, state will remain the same + return null; + } + render() { const { ariaLabelInput, @@ -510,13 +548,13 @@ export default class Slider extends PureComponent { disabled, name, light, - invalid, ...other } = this.props; delete other.onRelease; + delete other.invalid; - const { value, left } = this.state; + const { value, left, isValid } = this.state; const scope = this.context; let enabled; @@ -541,7 +579,7 @@ export default class Slider extends PureComponent { `${prefix}--slider-text-input`, { [`${prefix}--text-input--light`]: light, - [`${prefix}--text-input--invalid`]: this.props.invalid, + [`${prefix}--text-input--invalid`]: isValid === false, } ); @@ -579,7 +617,7 @@ export default class Slider extends PureComponent { onKeyDown={this.onKeyDown} role="presentation" tabIndex={-1} - data-invalid={invalid || null} + data-invalid={isValid ? null : true} {...other}>
diff --git a/packages/styles/scss/components/slider/_slider.scss b/packages/styles/scss/components/slider/_slider.scss index 4152279a9b71..35b83349914b 100644 --- a/packages/styles/scss/components/slider/_slider.scss +++ b/packages/styles/scss/components/slider/_slider.scss @@ -135,6 +135,10 @@ } } + .#{$prefix}--slider-text-input.#{$prefix}--text-input--invalid { + padding-right: 1rem; + } + .#{$prefix}--slider__thumb:focus ~ .#{$prefix}--slider__filled-track { background-color: $interactive; }