From 3e99fc7a9388999ee21ffa1b4f8b7cca200bc1d6 Mon Sep 17 00:00:00 2001 From: emyarod Date: Tue, 27 Aug 2019 14:39:59 -0500 Subject: [PATCH 1/3] feat(Tooltip): support controlled and uncontrolled modes (#3357) * refactor(Tooltip): rename `handleHover` and pass in full Event object * feat(Tooltip): set initial state based on controlled/uncontrolled mode * feat(Tooltip): handle user input on open/close actions * feat(Tooltip): use corresponding `open` value based on controlled mode * docs(Tooltip): add onChange propType behind feature flag * fix(Tooltip): remove unneeded default prop * fix(Tooltip): retain event reference in `handleMouse` * test(Tooltip): check if `open` prop is falsy * docs(Tooltip): add uncontrolled example --- .../src/components/Tooltip/Tooltip-story.js | 34 +++++++-- .../src/components/Tooltip/Tooltip-test.js | 6 +- .../react/src/components/Tooltip/Tooltip.js | 75 ++++++++++++++----- 3 files changed, 89 insertions(+), 26 deletions(-) diff --git a/packages/react/src/components/Tooltip/Tooltip-story.js b/packages/react/src/components/Tooltip/Tooltip-story.js index 2efc52e55976..2a63107bf3db 100644 --- a/packages/react/src/components/Tooltip/Tooltip-story.js +++ b/packages/react/src/components/Tooltip/Tooltip-story.js @@ -5,14 +5,12 @@ * LICENSE file in the root directory of this source tree. */ -import React from 'react'; +import React, { useState } from 'react'; import { storiesOf } from '@storybook/react'; import { settings } from 'carbon-components'; - import { withKnobs, select, text, number } from '@storybook/addon-knobs'; import Tooltip from '../Tooltip'; import Button from '../Button'; - import { OverflowMenuVertical16 } from '@carbon/icons-react'; const { prefix } = settings; @@ -22,7 +20,6 @@ const directions = { 'Top (top)': 'top', 'Right (right)': 'right', }; - const props = { withIcon: () => ({ direction: select('Tooltip direction (direction)', directions, 'bottom'), @@ -61,6 +58,32 @@ const props = { }), }; +function UncontrolledTooltipExample() { + const [value, setValue] = useState(true); + return ( + <> + + +
+ My text wrapped with tooltip
} + open={value} + showIcon={false}> + Some text + + + + ); +} + storiesOf('Tooltip', module) .addDecorator(withKnobs) .add( @@ -178,4 +201,5 @@ storiesOf('Tooltip', module) `, }, } - ); + ) + .add('uncontrolled tooltip', () => ); diff --git a/packages/react/src/components/Tooltip/Tooltip-test.js b/packages/react/src/components/Tooltip/Tooltip-test.js index fb9697719763..adaf8518bc59 100644 --- a/packages/react/src/components/Tooltip/Tooltip-test.js +++ b/packages/react/src/components/Tooltip/Tooltip-test.js @@ -226,7 +226,7 @@ describe('Tooltip', () => { const icon = wrapper.find(Information); icon.simulate('keyDown', { which: 'x' }); // Enzyme doesn't seem to allow state() in a forwardRef-wrapped class component - expect(wrapper.find('Tooltip').instance().state.open).toEqual(false); + expect(wrapper.find('Tooltip').instance().state.open).toBeFalsy(); }); it('A different key press does not change state when custom icon is set', () => { @@ -242,13 +242,13 @@ describe('Tooltip', () => { const icon = wrapper.find('.custom-icon'); icon.simulate('keyDown', { which: 'x' }); // Enzyme doesn't seem to allow state() in a forwardRef-wrapped class component - expect(wrapper.find('Tooltip').instance().state.open).toEqual(false); + expect(wrapper.find('Tooltip').instance().state.open).toBeFalsy(); }); it('should be in a closed state after handleOutsideClick() is invoked', () => { const rootWrapper = mount(); // Enzyme doesn't seem to allow state() in a forwardRef-wrapped class component - expect(rootWrapper.find('Tooltip').instance().state.open).toEqual(false); + expect(rootWrapper.find('Tooltip').instance().state.open).toBeFalsy(); // Enzyme doesn't seem to allow setState() in a forwardRef-wrapped class component rootWrapper .find('Tooltip') diff --git a/packages/react/src/components/Tooltip/Tooltip.js b/packages/react/src/components/Tooltip/Tooltip.js index 2c91357a99de..ad91102c25fc 100644 --- a/packages/react/src/components/Tooltip/Tooltip.js +++ b/packages/react/src/components/Tooltip/Tooltip.js @@ -22,6 +22,8 @@ import ClickListener from '../../internal/ClickListener'; import mergeRefs from '../../tools/mergeRefs'; import { keys, matches as keyDownMatch } from '../../internal/keyboard'; import isRequiredOneOf from '../../prop-types/isRequiredOneOf'; +import requiredIfValueExists from '../../prop-types/requiredIfValueExists'; +import { useControlledStateWithValue } from '../../internal/FeatureFlags'; const { prefix } = settings; @@ -73,7 +75,16 @@ const getMenuOffset = (menuBody, menuDirection) => { }; class Tooltip extends Component { - state = {}; + constructor(props) { + super(props); + this.isControlled = props.open !== undefined; + if (useControlledStateWithValue && this.isControlled) { + // Skips the logic of setting initial state if this component is controlled + return; + } + const open = useControlledStateWithValue ? props.defaultOpen : props.open; + this.state = { open }; + } static propTypes = { /** @@ -86,6 +97,11 @@ class Tooltip extends Component { */ tooltipId: PropTypes.string, + /** + * Optional starting value for uncontrolled state + */ + defaultOpen: PropTypes.bool, + /** * Open/closed state. */ @@ -162,10 +178,19 @@ class Tooltip extends Component { * Optional prop to specify the tabIndex of the Tooltip */ tabIndex: PropTypes.number, + + /** + * * the signature of the event handler will be: + * * `onChange(event, { open })` where: + * * `event` is the (React) raw event + * * `open` is the new value + */ + onChange: !useControlledStateWithValue + ? PropTypes.func + : requiredIfValueExists(PropTypes.func), }; static defaultProps = { - open: false, direction: DIRECTION_BOTTOM, renderIcon: Information, showIcon: true, @@ -182,7 +207,7 @@ class Tooltip extends Component { componentDidMount() { if (!this._debouncedHandleFocus) { - this._debouncedHandleFocus = debounce(this._handleHover, 200); + this._debouncedHandleFocus = debounce(this._handleFocus, 200); } requestAnimationFrame(() => { this.getTriggerPosition(); @@ -213,6 +238,14 @@ class Tooltip extends Component { }; } + _handleUserInputOpenClose = (event, { open }) => { + this.setState({ open }, () => { + if (this.props.onChange) { + this.props.onChange(event, { open }); + } + }); + }; + getTriggerPosition = () => { if (this.triggerEl) { const triggerPosition = this.triggerEl.getBoundingClientRect(); @@ -221,14 +254,15 @@ class Tooltip extends Component { }; /** - * Handles `mouseover`/`mouseout`/`focus`/`blur` event. + * Handles `focus`/`blur` event. * @param {string} state `over` to show the tooltip, `out` to hide the tooltip. - * @param {Element} [relatedTarget] For handing `mouseout` event, indicates where the mouse pointer is gone. + * @param {Element} [evt] For handing `mouseout` event, indicates where the mouse pointer is gone. */ - _handleHover = (state, relatedTarget) => { + _handleFocus = (state, evt) => { + const { relatedTarget } = evt; if (state === 'over') { this.getTriggerPosition(); - this.setState({ open: true }); + this._handleUserInputOpenClose(evt, { open: true }); } else { // Note: SVGElement in IE11 does not have `.contains()` const shouldPreventClose = @@ -238,7 +272,7 @@ class Tooltip extends Component { this.triggerEl.contains(relatedTarget)) || (this._tooltipEl && this._tooltipEl.contains(relatedTarget))); if (!shouldPreventClose) { - this.setState({ open: false }); + this._handleUserInputOpenClose(evt, { open: false }); } } }; @@ -259,6 +293,7 @@ class Tooltip extends Component { document.body; handleMouse = evt => { + evt.persist(); const state = { focus: 'over', blur: 'out', @@ -268,17 +303,19 @@ class Tooltip extends Component { this._hasContextMenu = evt.type === 'contextmenu'; if (state === 'click') { evt.stopPropagation(); - const shouldOpen = !this.state.open; + const shouldOpen = this.isControlled + ? !this.props.open + : !this.state.open; if (shouldOpen) { this.getTriggerPosition(); } - this.setState({ open: shouldOpen }); + this._handleUserInputOpenClose(evt, { open: shouldOpen }); } else if ( state && (state !== 'out' || !hadContextMenu) && this._debouncedHandleFocus ) { - this._debouncedHandleFocus(state, evt.relatedTarget); + this._debouncedHandleFocus(state, evt); } }; @@ -289,30 +326,32 @@ class Tooltip extends Component { this._tooltipEl && this._tooltipEl.contains(evt.target); if (!shouldPreventClose) { - this.setState({ open: false }); + this._handleUserInputOpenClose(evt, { open: false }); } }; handleKeyPress = event => { if (keyDownMatch(event, [keys.Escape])) { event.stopPropagation(); - this.setState({ open: false }); + this._handleUserInputOpenClose(event, { open: false }); } if (keyDownMatch(event, [keys.Enter, keys.Space])) { event.stopPropagation(); - const shouldOpen = !this.state.open; + const shouldOpen = this.isControlled + ? !this.props.open + : !this.state.open; if (shouldOpen) { this.getTriggerPosition(); } - this.setState({ open: shouldOpen }); + this._handleUserInputOpenClose(event, { open: shouldOpen }); } }; handleEscKeyPress = event => { - const { open } = this.state; + const { open } = this.isControlled ? this.props : this.state; if (open && keyDownMatch(event, [keys.Escape])) { - return this.setState({ open: false }); + return this._handleUserInputOpenClose(event, { open: false }); } }; @@ -343,7 +382,7 @@ class Tooltip extends Component { ...other } = this.props; - const { open } = this.state; + const { open } = this.isControlled ? this.props : this.state; const tooltipClasses = classNames( `${prefix}--tooltip`, From 151b18e70dac3739faa90b7bc2663ea57912a4b6 Mon Sep 17 00:00:00 2001 From: Akira Sudoh Date: Wed, 28 Aug 2019 05:00:27 +0900 Subject: [PATCH 2/3] fix(TableToolbarSearch): support back-tab (#3836) This change fixes the issue with `` that happens when the `` in it has focus and user performs back-tab gesture (Shift-Tab key). In such scenario (in before-change version), the root element of `` (`
`) gets focus given it has `tabindex="0"`, and the `onFocus()` handler of the element attempts to send focus back to the `` no matter what. This change also fixes the following found in the debugging effort: * An issue where `id` of `` in `` is regenerated for every render * Issues focus on `` happening: * For every render, regardless of change in expanded state * Even for controlled change in expanded state (without user gesture) Fixes #3762. --- .../components/data-table/_data-table-action.scss | 5 +++++ .../src/components/DataTable/TableToolbarSearch.js | 13 +++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/components/src/components/data-table/_data-table-action.scss b/packages/components/src/components/data-table/_data-table-action.scss index ac7fced063d3..0b71c9637a9c 100644 --- a/packages/components/src/components/data-table/_data-table-action.scss +++ b/packages/components/src/components/data-table/_data-table-action.scss @@ -230,6 +230,11 @@ .#{$prefix}--toolbar-action:focus:not([disabled]), .#{$prefix}--toolbar-action:active:not([disabled]) { @include focus-outline('outline'); + + &.#{$prefix}--toolbar-search-container-expandable { + // The focus style is handled by search input in it, need to avoid duplicate animation + outline: none; + } } .#{$prefix}--toolbar-action ~ .#{$prefix}--btn { diff --git a/packages/react/src/components/DataTable/TableToolbarSearch.js b/packages/react/src/components/DataTable/TableToolbarSearch.js index 8cc263c0f248..18d1c892262c 100644 --- a/packages/react/src/components/DataTable/TableToolbarSearch.js +++ b/packages/react/src/components/DataTable/TableToolbarSearch.js @@ -7,7 +7,7 @@ import cx from 'classnames'; import PropTypes from 'prop-types'; -import React, { useRef, useState, useEffect } from 'react'; +import React, { useMemo, useRef, useState, useEffect } from 'react'; import { settings } from 'carbon-components'; import Search from '../Search'; import setupGetInstanceId from './tools/instanceId'; @@ -34,7 +34,7 @@ const TableToolbarSearch = ({ onExpand, persistent, persistant, - id = `data-table-search-${getInstanceId()}`, + id, ...rest }) => { const { current: controlled } = useRef(expandedProp !== undefined); @@ -42,12 +42,13 @@ const TableToolbarSearch = ({ const expanded = controlled ? expandedProp : expandedState; const searchRef = useRef(null); const [value, setValue] = useState(''); + const uniqueId = useMemo(getInstanceId, []); useEffect(() => { - if (searchRef.current) { + if (!controlled && expandedState && searchRef.current) { searchRef.current.querySelector('input').focus(); } - }); + }, [controlled, expandedState]); const searchContainerClasses = cx({ [searchContainerClass]: searchContainerClass, @@ -77,7 +78,7 @@ const TableToolbarSearch = ({ return (
handleExpand(event, true)} @@ -89,7 +90,7 @@ const TableToolbarSearch = ({ small className={className} value={value} - id={id} + id={typeof id !== 'undefined' ? id : uniqueId} aria-hidden={!expanded} labelText={labelText || t('carbon.table.toolbar.search.label')} placeHolderText={ From 6c1221b7a3d9f52ccb0a28a44262161b7b41aa00 Mon Sep 17 00:00:00 2001 From: carbon-bot Date: Tue, 27 Aug 2019 20:15:16 +0000 Subject: [PATCH 3/3] chore(project): sync generated files [skip ci] --- packages/components/docs/sass.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/components/docs/sass.md b/packages/components/docs/sass.md index e52cfbccd568..7c9f1c4e191f 100644 --- a/packages/components/docs/sass.md +++ b/packages/components/docs/sass.md @@ -10303,6 +10303,11 @@ Data table action styles .#{$prefix}--toolbar-action:focus:not([disabled]), .#{$prefix}--toolbar-action:active:not([disabled]) { @include focus-outline('outline'); + + &.#{$prefix}--toolbar-search-container-expandable { + // The focus style is handled by search input in it, need to avoid duplicate animation + outline: none; + } } .#{$prefix}--toolbar-action ~ .#{$prefix}--btn {