From 8ba9ce443379cf1a06dcbf9627974ba358b1fef1 Mon Sep 17 00:00:00 2001 From: emyarod <emyarod@users.noreply.github.com> Date: Mon, 12 Apr 2021 12:26:31 -0500 Subject: [PATCH] fix(SelectableTile): sync selected prop and state (#8279) * docs(Tile): update deprecation warning * fix(Tile): sync props and state when uncontrolled * test(Tile): update SelectableTile tests * chore: update snapshots * docs(Tile): add temp example story * refactor(Tile): use hoisted functions instead of expressions * docs(Tile): remove test story Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --- .../__snapshots__/PublicAPI-test.js.snap | 17 +- .../react/src/components/Tile/Tile-test.js | 40 +- packages/react/src/components/Tile/Tile.js | 396 +++++++++--------- 3 files changed, 227 insertions(+), 226 deletions(-) diff --git a/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap b/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap index fcc6ac5b20e7..9b46f5ddb8ff 100644 --- a/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap +++ b/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap @@ -5836,10 +5836,7 @@ Map { }, "SelectableTile" => Object { "defaultProps": Object { - "handleClick": [Function], - "handleKeyDown": [Function], "light": false, - "onChange": [Function], "selected": false, "tabIndex": 0, "title": "title", @@ -5855,12 +5852,8 @@ Map { "disabled": Object { "type": "bool", }, - "handleClick": Object { - "type": "func", - }, - "handleKeyDown": Object { - "type": "func", - }, + "handleClick": [Function], + "handleKeyDown": [Function], "iconDescription": [Function], "id": Object { "type": "string", @@ -5874,6 +5867,12 @@ Map { "onChange": Object { "type": "func", }, + "onClick": Object { + "type": "func", + }, + "onKeyDown": Object { + "type": "func", + }, "selected": Object { "type": "bool", }, diff --git a/packages/react/src/components/Tile/Tile-test.js b/packages/react/src/components/Tile/Tile-test.js index 8133f16117aa..73327ccefac7 100644 --- a/packages/react/src/components/Tile/Tile-test.js +++ b/packages/react/src/components/Tile/Tile-test.js @@ -134,11 +134,10 @@ describe('Tile', () => { beforeEach(() => { wrapper = mount( - <SelectableTile className="extra-class"> + <SelectableTile className="extra-class" onClick={jest.fn()}> <div className="child">Test</div> </SelectableTile> ); - wrapper.state().selected = false; label = wrapper.find('label'); }); @@ -155,42 +154,51 @@ describe('Tile', () => { }); it('toggles the selectable state on click', () => { - expect(wrapper.state().selected).toEqual(false); + expect(wrapper.hasClass(`${prefix}--tile--is-selected`)).toEqual(false); label.simulate('click'); - expect(wrapper.state().selected).toEqual(true); + expect(wrapper.props().onClick).toHaveBeenCalledTimes(1); + expect(wrapper.render().hasClass(`${prefix}--tile--is-selected`)).toEqual( + true + ); }); it('toggles the selectable state when using enter or space', () => { - expect(wrapper.state().selected).toEqual(false); + expect(wrapper.hasClass(`${prefix}--tile--is-selected`)).toEqual(false); label.simulate('keydown', { which: 32 }); - expect(wrapper.state().selected).toEqual(true); + expect(wrapper.render().hasClass(`${prefix}--tile--is-selected`)).toEqual( + true + ); label.simulate('keydown', { which: 13 }); - expect(wrapper.state().selected).toEqual(false); + expect(wrapper.render().hasClass(`${prefix}--tile--is-selected`)).toEqual( + false + ); }); it('the input should be checked when state is selected', () => { - wrapper.setState({ selected: true }); + label.simulate('click'); expect(wrapper.find('input').props().checked).toEqual(true); }); it('supports setting initial selected state from props', () => { - expect(shallow(<SelectableTile selected />).state().selected).toEqual( - true - ); + expect( + shallow(<SelectableTile selected />) + .render() + .hasClass(`${prefix}--tile--is-selected`) + ).toEqual(true); }); it('supports setting selected state from props', () => { wrapper.setProps({ selected: true }); - wrapper.setState({ selected: true }); - wrapper.setProps({ selected: false }); - expect(wrapper.state().selected).toEqual(false); + expect(wrapper.render().hasClass(`${prefix}--tile--is-selected`)).toEqual( + true + ); }); it('avoids changing selected state upon setting props, unless actual value change is detected', () => { wrapper.setProps({ selected: true }); - wrapper.setState({ selected: false }); + label.simulate('click'); wrapper.setProps({ selected: true }); - expect(wrapper.state().selected).toEqual(false); + expect(wrapper.hasClass(`${prefix}--tile--is-selected`)).toEqual(false); }); it('supports light version', () => { diff --git a/packages/react/src/components/Tile/Tile.js b/packages/react/src/components/Tile/Tile.js index 83c39b07494d..ebd7134cc9a5 100644 --- a/packages/react/src/components/Tile/Tile.js +++ b/packages/react/src/components/Tile/Tile.js @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. */ -import React, { Component } from 'react'; +import React, { Component, useEffect, useRef, useState } from 'react'; import PropTypes from 'prop-types'; import classNames from 'classnames'; import { settings } from 'carbon-components'; @@ -181,216 +181,210 @@ export class ClickableTile extends Component { } } -export class SelectableTile extends Component { - state = { - selected: this.props.selected, - }; - - static propTypes = { - /** - * The child nodes. - */ - children: PropTypes.node, - - /** - * The CSS class names. - */ - className: PropTypes.string, - - /** - * Specify whether the SelectableTile should be disabled - */ - disabled: PropTypes.bool, - - /** - * Specify the function to run when the SelectableTile is clicked - */ - handleClick: PropTypes.func, - - /** - * Specify the function to run when the SelectableTile is interacted with via a keyboard - */ - handleKeyDown: PropTypes.func, - - /** - * The description of the checkmark icon. - */ - iconDescription: deprecate( - PropTypes.string, - 'The `iconDescription` prop for `RadioTile` is no longer needed and has ' + - 'been deprecated. It will be moved in the next major release.' - ), - - /** - * The ID of the `<input>`. - */ - id: PropTypes.string, - - /** - * `true` to use the light version. For use on $ui-01 backgrounds only. - * Don't use this to make tile background color same as container background color. - */ - light: PropTypes.bool, - - /** - * The `name` of the `<input>`. - */ - name: PropTypes.string, - - /** - * The empty handler of the `<input>`. - */ - onChange: PropTypes.func, - - /** - * `true` to select this tile. - */ - selected: PropTypes.bool, - - /** - * Specify the tab index of the wrapper element - */ - tabIndex: PropTypes.number, - - /** - * The `title` of the `<input>`. - */ - title: PropTypes.string, - - /** - * The value of the `<input>`. - */ - value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired, - }; - - static defaultProps = { - value: 'value', - title: 'title', - selected: false, - handleClick: () => {}, - handleKeyDown: () => {}, - onChange: () => {}, - tabIndex: 0, - light: false, - }; - - static getDerivedStateFromProps({ selected }, state) { - const { prevSelected } = state; - return prevSelected === selected - ? null - : { - selected, - prevSelected: selected, - }; - } - - handleClick = (evt) => { +export function SelectableTile(props) { + const { + children, + id, + tabIndex = 0, + value, + name, + title, + // eslint-disable-next-line no-unused-vars + iconDescription, + className, + handleClick, + handleKeyDown, + onClick = () => {}, + onChange = () => {}, + onKeyDown = () => {}, + light, + disabled, + selected, + ...other + } = props; + + // TODO: replace with onClick when handleClick prop is deprecated + const clickHandler = handleClick || onClick; + + // TODO: replace with onClick when handleClick prop is deprecated + const keyDownHandler = handleKeyDown || onKeyDown; + + const [isSelected, setIsSelected] = useState(selected); + const input = useRef(null); + const classes = classNames( + `${prefix}--tile`, + `${prefix}--tile--selectable`, + { + [`${prefix}--tile--is-selected`]: isSelected, + [`${prefix}--tile--light`]: light, + [`${prefix}--tile--disabled`]: disabled, + }, + className + ); + const inputClasses = classNames(`${prefix}--tile-input`, { + [`${prefix}--tile-input--checked`]: isSelected, + }); + + // TODO: rename to handleClick when handleClick prop is deprecated + function handleOnClick(evt) { evt.preventDefault(); evt.persist(); - this.setState( - { - selected: !this.state.selected, - }, - () => { - this.props.handleClick(evt); - this.props.onChange(evt); - } - ); - }; + setIsSelected(!isSelected); + clickHandler(evt); + onChange(evt); + } - handleKeyDown = (evt) => { + // TODO: rename to handleKeyDown when handleKeyDown prop is deprecated + function handleOnKeyDown(evt) { evt.persist(); if (matches(evt, [keys.Enter, keys.Space])) { evt.preventDefault(); - this.setState( - { - selected: !this.state.selected, - }, - () => { - this.props.handleKeyDown(evt); - this.props.onChange(evt); - } - ); - } else { - this.props.handleKeyDown(evt); + setIsSelected(!isSelected); + onChange(evt); } - }; - - handleOnChange = (event) => { - this.setState({ selected: event.target.checked }); - this.props.onChange(event); - }; - - render() { - const { - children, - id, - tabIndex, - value, - name, - title, - // eslint-disable-next-line no-unused-vars - iconDescription, - className, - handleClick, // eslint-disable-line - handleKeyDown, // eslint-disable-line - // eslint-disable-next-line no-unused-vars - onChange, - light, - disabled, - ...other - } = this.props; - - const inputClasses = classNames(`${prefix}--tile-input`, { - [`${prefix}--tile-input--checked`]: this.state.selected, - }); - - const classes = classNames( - `${prefix}--tile`, - `${prefix}--tile--selectable`, - { - [`${prefix}--tile--is-selected`]: this.state.selected, - [`${prefix}--tile--light`]: light, - [`${prefix}--tile--disabled`]: disabled, - }, - className - ); + keyDownHandler(evt); + } - return ( - <> - <input - ref={(input) => { - this.input = input; - }} - tabIndex={-1} - id={id} - className={inputClasses} - value={value} - onChange={!disabled ? this.handleOnChange : null} - type="checkbox" - disabled={disabled} - name={name} - title={title} - checked={this.state.selected} - /> - {/* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions */} - <label - htmlFor={id} - className={classes} - // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex - tabIndex={!disabled ? tabIndex : null} - {...other} - onClick={!disabled ? this.handleClick : null} - onKeyDown={!disabled ? this.handleKeyDown : null}> - <span className={`${prefix}--tile__checkmark`}> - <CheckmarkFilled /> - </span> - <span className={`${prefix}--tile-content`}>{children}</span> - </label> - </> - ); + function handleChange(event) { + setIsSelected(event.target.checked); + onChange(event); } + + useEffect(() => { + setIsSelected(selected); + }, [selected]); + + return ( + <> + <input + ref={input} + tabIndex={-1} + id={id} + className={inputClasses} + value={value} + onChange={!disabled ? handleChange : null} + type="checkbox" + disabled={disabled} + name={name} + title={title} + checked={isSelected} + /> + {/* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions */} + <label + htmlFor={id} + className={classes} + // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex + tabIndex={!disabled ? tabIndex : null} + {...other} + onClick={!disabled ? handleOnClick : null} + onKeyDown={!disabled ? handleOnKeyDown : null}> + <span className={`${prefix}--tile__checkmark`}> + <CheckmarkFilled /> + </span> + <span className={`${prefix}--tile-content`}>{children}</span> + </label> + </> + ); } +SelectableTile.defaultProps = { + value: 'value', + title: 'title', + selected: false, + tabIndex: 0, + light: false, +}; +SelectableTile.propTypes = { + /** + * The child nodes. + */ + children: PropTypes.node, + + /** + * The CSS class names. + */ + className: PropTypes.string, + + /** + * Specify whether the SelectableTile should be disabled + */ + disabled: PropTypes.bool, + + /** + * Specify the function to run when the SelectableTile is clicked + */ + handleClick: deprecate( + PropTypes.func, + 'The `handleClick` prop for `SelectableTile` has been deprecated in favor of `onClick`. It will be removed in the next major release.' + ), + + /** + * Specify the function to run when the SelectableTile is interacted with via a keyboard + */ + handleKeyDown: deprecate( + PropTypes.func, + 'The `handleKeyDown` prop for `SelectableTile` has been deprecated in favor of `onKeyDown`. It will be removed in the next major release.' + ), + + /** + * The description of the checkmark icon. + */ + iconDescription: deprecate( + PropTypes.string, + 'The `iconDescription` prop for `SelectableTile` is no longer needed and has ' + + 'been deprecated. It will be removed in the next major release.' + ), + + /** + * The ID of the `<input>`. + */ + id: PropTypes.string, + + /** + * `true` to use the light version. For use on $ui-01 backgrounds only. + * Don't use this to make tile background color same as container background color. + */ + light: PropTypes.bool, + + /** + * The `name` of the `<input>`. + */ + name: PropTypes.string, + + /** + * The empty handler of the `<input>`. + */ + onChange: PropTypes.func, + + /** + * Specify the function to run when the SelectableTile is clicked + */ + onClick: PropTypes.func, + + /** + * Specify the function to run when the SelectableTile is interacted with via a keyboard + */ + onKeyDown: PropTypes.func, + + /** + * `true` to select this tile. + */ + selected: PropTypes.bool, + + /** + * Specify the tab index of the wrapper element + */ + tabIndex: PropTypes.number, + + /** + * The `title` of the `<input>`. + */ + title: PropTypes.string, + + /** + * The value of the `<input>`. + */ + value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired, +}; export class ExpandableTile extends Component { state = {};