From 2de6ce09c9bfd6ad807d5aa4c0d25613ecaf5d69 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 7 Apr 2019 13:17:16 +0100 Subject: [PATCH 01/11] [Tooltip] Convert to function component --- packages/material-ui/src/Tooltip/Tooltip.js | 417 +++++++++--------- .../material-ui/src/Tooltip/Tooltip.test.js | 143 +++--- 2 files changed, 266 insertions(+), 294 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index 3f1aa65fb28dd5..fc8ca342c9ce93 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -72,28 +72,49 @@ export const styles = theme => ({ }, }); -class Tooltip extends React.Component { - ignoreNonTouchEvents = false; - - constructor(props) { - super(); - this.isControlled = props.open != null; - this.state = { - open: null, - }; - - if (!this.isControlled) { - // not controlled, use internal state - this.state.open = false; - } - } - - componentDidMount() { +function Tooltip(props) { + const { + children, + classes, + disableFocusListener, + disableHoverListener, + disableTouchListener, + enterDelay, + enterTouchDelay, + id, + interactive, + leaveDelay, + leaveTouchDelay, + onClose, + onOpen, + open: openProp, + placement, + PopperProps, + theme, + title, + TransitionComponent, + TransitionProps, + ...other + } = props; + + const ignoreNonTouchEvents = React.useRef(false); + const { current: isControlled } = React.useRef(props.open != null); + const childrenRef = React.useRef(); + const defaultId = React.useRef(); + const isMounted = React.useRef(false); + const closeTimer = React.useRef(); + const enterTimer = React.useRef(); + const leaveTimer = React.useRef(); + const touchTimer = React.useRef(); + const [, forceUpdate] = React.useState(); + const [openState, setOpenState] = React.useState(false); + + React.useEffect(() => { warning( - !this.childrenRef.disabled || - (this.childrenRef.disabled && this.isControlled) || - (this.childrenRef.disabled && this.props.title === '') || - this.childrenRef.tagName.toLowerCase() !== 'button', + !childrenRef.current.disabled || + (childrenRef.current.disabled && isControlled) || + (childrenRef.current.disabled && title === '') || + childrenRef.current.tagName.toLowerCase() !== 'button', [ 'Material-UI: you are providing a disabled `button` child to the Tooltip component.', 'A disabled element does not fire events.', @@ -106,86 +127,101 @@ class Tooltip extends React.Component { // Fallback to this default id when possible. // Use the random value for client side rendering only. // We can't use it server-side. - this.defaultId = `mui-tooltip-${Math.round(Math.random() * 1e5)}`; + defaultId.current = `mui-tooltip-${Math.round(Math.random() * 1e5)}`; - // Rerender with this.defaultId and this.childrenRef. - if (this.props.open) { - this.forceUpdate(); + // Rerender with defaultId and childrenRef. + if (openProp && !isMounted.current) { + forceUpdate(); } - } - - componentWillUnmount() { - clearTimeout(this.closeTimer); - clearTimeout(this.enterTimer); - clearTimeout(this.focusTimer); - clearTimeout(this.leaveTimer); - clearTimeout(this.touchTimer); - } + isMounted.current = true; + }, [isControlled, title, openProp]); + + React.useEffect(() => { + return () => { + clearTimeout(closeTimer.current); + clearTimeout(enterTimer.current); + clearTimeout(leaveTimer.current); + clearTimeout(touchTimer.current); + }; + }); - onRootRef = ref => { - this.childrenRef = ref; + const onRootRef = ref => { + childrenRef.current = ref; }; - handleFocus = event => { - // Workaround for https://github.com/facebook/react/issues/7769 - // The autoFocus of React might trigger the event before the componentDidMount. - // We need to account for this eventuality. - if (!this.childrenRef) { - this.childrenRef = event.currentTarget; + const handleOpen = event => { + // The mouseover event will trigger for every nested element in the tooltip. + // We can skip rerendering when the tooltip is already open. + // We are using the mouseover event instead of the mouseenter event to fix a hide/show issue. + if (!isControlled && !openState) { + setOpenState(true); } - this.handleEnter(event); - - const childrenProps = this.props.children.props; - if (childrenProps.onFocus) { - childrenProps.onFocus(event); + if (onOpen) { + onOpen(event); } }; - handleEnter = event => { - const { children, enterDelay } = this.props; + const handleEnter = event => { const childrenProps = children.props; if (event.type === 'mouseover' && childrenProps.onMouseOver) { childrenProps.onMouseOver(event); } - if (this.ignoreNonTouchEvents && event.type !== 'touchstart') { + if (ignoreNonTouchEvents.current && event.type !== 'touchstart') { return; } // Remove the title ahead of time. // We don't want to wait for the next render commit. // We would risk displaying two tooltips at the same time (native + this one). - this.childrenRef.setAttribute('title', ''); + childrenRef.current.setAttribute('title', ''); - clearTimeout(this.enterTimer); - clearTimeout(this.leaveTimer); + clearTimeout(enterTimer.current); + clearTimeout(leaveTimer.current); if (enterDelay) { event.persist(); - this.enterTimer = setTimeout(() => { - this.handleOpen(event); + enterTimer.current = setTimeout(() => { + handleOpen(event); }, enterDelay); } else { - this.handleOpen(event); + handleOpen(event); } }; - handleOpen = event => { - // The mouseover event will trigger for every nested element in the tooltip. - // We can skip rerendering when the tooltip is already open. - // We are using the mouseover event instead of the mouseenter event to fix a hide/show issue. - if (!this.isControlled && !this.state.open) { - this.setState({ open: true }); + const handleFocus = event => { + // Workaround for https://github.com/facebook/react/issues/7769 + // The autoFocus of React might trigger the event before the componentDidMount. + // We need to account for this eventuality. + if (!childrenRef.current) { + childrenRef.current = event.currentTarget; } - if (this.props.onOpen) { - this.props.onOpen(event); + handleEnter(event); + + const childrenProps = children.props; + if (childrenProps.onFocus) { + childrenProps.onFocus(event); } }; - handleLeave = event => { - const { children, leaveDelay } = this.props; + const handleClose = event => { + if (!isControlled) { + setOpenState(false); + } + + if (onClose) { + onClose(event); + } + + clearTimeout(closeTimer.current); + closeTimer.current = setTimeout(() => { + ignoreNonTouchEvents.current = false; + }, theme.transitions.duration.shortest); + }; + + const handleLeave = event => { const childrenProps = children.props; if (event.type === 'blur' && childrenProps.onBlur) { @@ -196,181 +232,138 @@ class Tooltip extends React.Component { childrenProps.onMouseLeave(event); } - clearTimeout(this.enterTimer); - clearTimeout(this.leaveTimer); + clearTimeout(enterTimer.current); + clearTimeout(leaveTimer.current); if (leaveDelay) { event.persist(); - this.leaveTimer = setTimeout(() => { - this.handleClose(event); + leaveTimer.current = setTimeout(() => { + handleClose(event); }, leaveDelay); } else { - this.handleClose(event); + handleClose(event); } }; - handleClose = event => { - if (!this.isControlled) { - this.setState({ open: false }); - } - - if (this.props.onClose) { - this.props.onClose(event); - } - - clearTimeout(this.closeTimer); - this.closeTimer = setTimeout(() => { - this.ignoreNonTouchEvents = false; - }, this.props.theme.transitions.duration.shortest); - }; - - handleTouchStart = event => { - this.ignoreNonTouchEvents = true; - const { children, enterTouchDelay } = this.props; + const handleTouchStart = event => { + ignoreNonTouchEvents.current = true; + const childrenProps = children.props; - if (children.props.onTouchStart) { - children.props.onTouchStart(event); + if (childrenProps.onTouchStart) { + childrenProps.onTouchStart(event); } - clearTimeout(this.leaveTimer); - clearTimeout(this.closeTimer); - clearTimeout(this.touchTimer); + clearTimeout(leaveTimer.current); + clearTimeout(closeTimer.current); + clearTimeout(touchTimer.current); event.persist(); - this.touchTimer = setTimeout(() => { - this.handleEnter(event); + touchTimer.current = setTimeout(() => { + handleEnter(event); }, enterTouchDelay); }; - handleTouchEnd = event => { - const { children, leaveTouchDelay } = this.props; - + const handleTouchEnd = event => { if (children.props.onTouchEnd) { children.props.onTouchEnd(event); } - clearTimeout(this.touchTimer); - clearTimeout(this.leaveTimer); + clearTimeout(touchTimer.current); + clearTimeout(leaveTimer.current); event.persist(); - this.leaveTimer = setTimeout(() => { - this.handleClose(event); + leaveTimer.current = setTimeout(() => { + handleClose(event); }, leaveTouchDelay); }; - render() { - const { - children, - classes, - disableFocusListener, - disableHoverListener, - disableTouchListener, - enterDelay, - enterTouchDelay, - id, - interactive, - leaveDelay, - leaveTouchDelay, - onClose, - onOpen, - open: openProp, - placement, - PopperProps, - theme, - title, - TransitionComponent, - TransitionProps, - ...other - } = this.props; - - let open = this.isControlled ? openProp : this.state.open; - - // There is no point in displaying an empty tooltip. - if (title === '') { - open = false; - } - - // For accessibility and SEO concerns, we render the title to the DOM node when - // the tooltip is hidden. However, we have made a tradeoff when - // `disableHoverListener` is set. This title logic is disabled. - // It's allowing us to keep the implementation size minimal. - // We are open to change the tradeoff. - const shouldShowNativeTitle = !open && !disableHoverListener; - const childrenProps = { - 'aria-describedby': open ? id || this.defaultId : null, - title: shouldShowNativeTitle && typeof title === 'string' ? title : null, - ...other, - ...children.props, - className: clsx(other.className, children.props.className), - }; + let open = isControlled ? openProp : openState; - if (!disableTouchListener) { - childrenProps.onTouchStart = this.handleTouchStart; - childrenProps.onTouchEnd = this.handleTouchEnd; - } + // There is no point in displaying an empty tooltip. + if (title === '') { + open = false; + } - if (!disableHoverListener) { - childrenProps.onMouseOver = this.handleEnter; - childrenProps.onMouseLeave = this.handleLeave; - } + // For accessibility and SEO concerns, we render the title to the DOM node when + // the tooltip is hidden. However, we have made a tradeoff when + // `disableHoverListener` is set. This title logic is disabled. + // It's allowing us to keep the implementation size minimal. + // We are open to change the tradeoff. + const shouldShowNativeTitle = !open && !disableHoverListener; + const childrenProps = { + 'aria-describedby': open ? id || defaultId.current : null, + title: shouldShowNativeTitle && typeof title === 'string' ? title : null, + ...other, + ...children.props, + className: clsx(other.className, children.props.className), + }; - if (!disableFocusListener) { - childrenProps.onFocus = this.handleFocus; - childrenProps.onBlur = this.handleLeave; - } + if (!disableTouchListener) { + childrenProps.onTouchStart = handleTouchStart; + childrenProps.onTouchEnd = handleTouchEnd; + } - const interactiveWrapperListeners = interactive - ? { - onMouseOver: childrenProps.onMouseOver, - onMouseLeave: childrenProps.onMouseLeave, - onFocus: childrenProps.onFocus, - onBlur: childrenProps.onBlur, - } - : {}; + if (!disableHoverListener) { + childrenProps.onMouseOver = handleEnter; + childrenProps.onMouseLeave = handleLeave; + } - warning( - !children.props.title, - [ - 'Material-UI: you have provided a `title` property to the child of .', - `Remove this title property \`${children.props.title}\` or the Tooltip component.`, - ].join('\n'), - ); + if (!disableFocusListener) { + childrenProps.onFocus = handleFocus; + childrenProps.onBlur = handleLeave; + } - return ( - - {React.cloneElement(children, childrenProps)} - - {({ placement: placementInner, TransitionProps: TransitionPropsInner }) => ( - .', + `Remove this title property \`${children.props.title}\` or the Tooltip component.`, + ].join('\n'), + ); + + return ( + + {React.cloneElement(children, childrenProps)} + + {({ placement: placementInner, TransitionProps: TransitionPropsInner }) => ( + +
-
- {title} -
- - )} - - - ); - } + {title} +
+
+ )} +
+
+ ); } Tooltip.propTypes = { diff --git a/packages/material-ui/src/Tooltip/Tooltip.test.js b/packages/material-ui/src/Tooltip/Tooltip.test.js index 43ed983f8cce45..1eef0468f13a49 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.test.js +++ b/packages/material-ui/src/Tooltip/Tooltip.test.js @@ -3,20 +3,17 @@ import { assert } from 'chai'; import PropTypes from 'prop-types'; import { spy, useFakeTimers } from 'sinon'; import consoleErrorMock from 'test/utils/consoleErrorMock'; -import { createShallow, createMount, getClasses, unwrap } from '@material-ui/core/test-utils'; +import { createMount, getClasses, unwrap } from '@material-ui/core/test-utils'; import Popper from '../Popper'; import Tooltip from './Tooltip'; import Input from '../Input'; import createMuiTheme from '../styles/createMuiTheme'; import RootRef from '../RootRef'; -function persist() {} - const TooltipNaked = unwrap(Tooltip); const theme = createMuiTheme(); describe('', () => { - let shallow; let mount; let classes; let clock; @@ -27,7 +24,6 @@ describe('', () => { }; before(() => { - shallow = createShallow({ dive: true, disableLifecycleMethods: true }); mount = createMount(); classes = getClasses(); clock = useFakeTimers(); @@ -39,16 +35,16 @@ describe('', () => { }); it('should render the correct structure', () => { - const wrapper = shallow(); - assert.strictEqual(wrapper.type(), React.Fragment); - assert.strictEqual(wrapper.childAt(0).type(), RootRef); - assert.strictEqual(wrapper.childAt(1).type(), Popper); - assert.strictEqual(wrapper.childAt(1).hasClass(classes.popper), true); + const wrapper = mount(); + const children = wrapper.childAt(0); + assert.strictEqual(children.childAt(0).type(), RootRef); + assert.strictEqual(children.childAt(1).type(), Popper); + assert.strictEqual(children.childAt(1).hasClass(classes.popper), true); }); describe('prop: disableHoverListener', () => { it('should hide the native title', () => { - const wrapper = shallow( + const wrapper = mount( , @@ -66,12 +62,12 @@ describe('', () => { }); it('should not display if the title is an empty string', () => { - const wrapper = shallow(); + const wrapper = mount(); assert.strictEqual(wrapper.find(Popper).props().open, false); }); it('should be passed down to the child as a native title', () => { - const wrapper = shallow( + const wrapper = mount( , @@ -84,88 +80,74 @@ describe('', () => { describe('prop: placement', () => { it('should have top placement', () => { - const wrapper = shallow(); + const wrapper = mount(); assert.strictEqual(wrapper.find(Popper).props().placement, 'top'); }); }); it('should respond to external events', () => { - const wrapper = shallow(); - wrapper.instance().childrenRef = document.createElement('div'); + const wrapper = mount(); const children = wrapper.childAt(0).childAt(0); - assert.strictEqual(wrapper.instance().state.open, false); - children.simulate('mouseOver', { type: 'mouseover' }); - assert.strictEqual(wrapper.instance().state.open, true); - children.simulate('mouseLeave', { type: 'mouseleave' }); - assert.strictEqual(wrapper.instance().state.open, false); + assert.strictEqual(wrapper.find(Popper).props().open, false); + children.simulate('mouseOver'); + assert.strictEqual(wrapper.find(Popper).props().open, true); + children.simulate('mouseLeave'); + assert.strictEqual(wrapper.find(Popper).props().open, false); }); it('should be controllable', () => { const handleRequestOpen = spy(); const handleClose = spy(); - const wrapper = shallow( + const wrapper = mount( , ); - wrapper.instance().childrenRef = document.createElement('div'); const children = wrapper.childAt(0).childAt(0); assert.strictEqual(handleRequestOpen.callCount, 0); assert.strictEqual(handleClose.callCount, 0); - children.simulate('mouseOver', { type: 'mouseover' }); + children.simulate('mouseOver'); assert.strictEqual(handleRequestOpen.callCount, 1); assert.strictEqual(handleClose.callCount, 0); - children.simulate('mouseLeave', { type: 'mouseleave' }); + children.simulate('mouseLeave'); assert.strictEqual(handleRequestOpen.callCount, 1); assert.strictEqual(handleClose.callCount, 1); }); it('should close when the interaction is over', () => { - const wrapper = shallow(); - const childrenRef = document.createElement('div'); - childrenRef.tabIndex = 0; - wrapper.instance().childrenRef = childrenRef; + const wrapper = mount(); const children = wrapper.childAt(0).childAt(0); - assert.strictEqual(wrapper.instance().state.open, false); - children.simulate('mouseOver', { type: 'mouseover' }); - childrenRef.focus(); - children.simulate('focus', { type: 'focus', persist }); - clock.tick(0); - assert.strictEqual(wrapper.instance().state.open, true); - children.simulate('mouseLeave', { type: 'mouseleave' }); - assert.strictEqual(wrapper.instance().state.open, false); - children.simulate('blur', { type: 'blur' }); - assert.strictEqual(wrapper.instance().state.open, false); + assert.strictEqual(wrapper.find(Popper).props().open, false); + children.simulate('mouseOver'); + children.simulate('focus'); + assert.strictEqual(wrapper.find(Popper).props().open, true); + children.simulate('mouseLeave'); + assert.strictEqual(wrapper.find(Popper).props().open, false); + children.simulate('blur'); + assert.strictEqual(wrapper.find(Popper).props().open, false); }); describe('touch screen', () => { it('should not respond to quick events', () => { - const wrapper = shallow(); - const childrenRef = document.createElement('div'); - childrenRef.tabIndex = 0; - wrapper.instance().childrenRef = childrenRef; + const wrapper = mount(); const children = wrapper.childAt(0).childAt(0); - children.simulate('touchStart', { type: 'touchstart', persist }); - children.simulate('touchEnd', { type: 'touchend', persist }); - childrenRef.focus(); - children.simulate('focus', { type: 'focus', persist }); - assert.strictEqual(wrapper.instance().state.open, false); + children.simulate('touchStart'); + children.simulate('touchEnd'); + children.simulate('focus'); + assert.strictEqual(wrapper.find(Popper).props().open, false); }); it('should open on long press', () => { - const wrapper = shallow(); - const childrenRef = document.createElement('div'); - childrenRef.tabIndex = 0; - wrapper.instance().childrenRef = childrenRef; + const wrapper = mount(); const children = wrapper.childAt(0).childAt(0); - children.simulate('touchStart', { type: 'touchstart', persist }); - childrenRef.focus(); - children.simulate('focus', { type: 'focus', persist }); - clock.tick(1e3); - assert.strictEqual(wrapper.instance().state.open, true); - children.simulate('touchEnd', { type: 'touchend', persist }); - children.simulate('blur', { type: 'blur' }); + children.simulate('touchStart'); + children.simulate('focus'); + clock.tick(1000); + wrapper.update(); + assert.strictEqual(wrapper.find(Popper).props().open, true); + children.simulate('touchEnd'); + children.simulate('blur'); clock.tick(1500); - assert.strictEqual(wrapper.instance().state.open, false); + assert.strictEqual(wrapper.find(Popper).props().open, false); }); }); @@ -200,27 +182,25 @@ describe('', () => { describe('prop: delay', () => { it('should take the enterDelay into account', () => { const wrapper = mount(); - const childrenRef = wrapper.instance().childrenRef; - childrenRef.tabIndex = 0; - childrenRef.focus(); - assert.strictEqual(document.activeElement, childrenRef); - assert.strictEqual(wrapper.instance().state.open, false); + const children = wrapper.childAt(0).childAt(0); + children.simulate('focus'); + assert.strictEqual(wrapper.find(Popper).props().open, false); clock.tick(111); - assert.strictEqual(wrapper.instance().state.open, true); + wrapper.update(); + assert.strictEqual(wrapper.find(Popper).props().open, true); }); it('should take the leaveDelay into account', () => { const wrapper = mount(); - const childrenRef = wrapper.instance().childrenRef; - childrenRef.tabIndex = 0; - childrenRef.focus(); - assert.strictEqual(document.activeElement, childrenRef); + const children = wrapper.childAt(0).childAt(0); + children.simulate('focus'); clock.tick(0); - assert.strictEqual(wrapper.instance().state.open, true); - childrenRef.blur(); - assert.strictEqual(wrapper.instance().state.open, true); + assert.strictEqual(wrapper.find(Popper).props().open, true); + children.simulate('blur'); + assert.strictEqual(wrapper.find(Popper).props().open, true); clock.tick(111); - assert.strictEqual(wrapper.instance().state.open, false); + wrapper.update(); + assert.strictEqual(wrapper.find(Popper).props().open, false); }); }); @@ -236,17 +216,16 @@ describe('', () => { ].forEach(name => { it(`should be transparent for the ${name} event`, () => { const handler = spy(); - const wrapper = shallow( + const wrapper = mount( , ); - wrapper.instance().childrenRef = document.createElement('div'); const children = wrapper.childAt(0).childAt(0); const type = name.slice(2).toLowerCase(); - children.simulate(type, { type, persist }); + children.simulate(type); clock.tick(0); assert.strictEqual(handler.callCount, 1); }); @@ -311,19 +290,19 @@ describe('', () => { const children = wrapper.childAt(0).childAt(0); children.simulate('mouseOver', { type: 'mouseOver' }); clock.tick(0); - assert.strictEqual(tooltipNaked.instance().state.open, true); + assert.strictEqual(wrapper.find(Popper).props().open, true); const popper = wrapper.find(Popper); children.simulate('mouseLeave', { type: 'mouseleave' }); - assert.strictEqual(tooltipNaked.instance().state.open, true); + assert.strictEqual(wrapper.find(Popper).props().open, true); popper.simulate('mouseOver', { type: 'mouseover' }); clock.tick(111); - assert.strictEqual(tooltipNaked.instance().state.open, true); + assert.strictEqual(wrapper.find(Popper).props().open, true); }); }); describe('forward', () => { it('should forward properties to the child element', () => { - const wrapper = shallow( + const wrapper = mount(

H1

, @@ -332,7 +311,7 @@ describe('', () => { }); it('should respect the properties priority', () => { - const wrapper = shallow( + const wrapper = mount( , From 3df2d60d3842981fe6a3e20fd04eb20c3efb36c6 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Tue, 9 Apr 2019 22:54:01 +0100 Subject: [PATCH 02/11] Remove unwrap usage --- packages/material-ui/src/Tooltip/Tooltip.test.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.test.js b/packages/material-ui/src/Tooltip/Tooltip.test.js index 1eef0468f13a49..625bb8031306dd 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.test.js +++ b/packages/material-ui/src/Tooltip/Tooltip.test.js @@ -3,14 +3,13 @@ import { assert } from 'chai'; import PropTypes from 'prop-types'; import { spy, useFakeTimers } from 'sinon'; import consoleErrorMock from 'test/utils/consoleErrorMock'; -import { createMount, getClasses, unwrap } from '@material-ui/core/test-utils'; +import { createMount, getClasses } from '@material-ui/core/test-utils'; import Popper from '../Popper'; import Tooltip from './Tooltip'; import Input from '../Input'; import createMuiTheme from '../styles/createMuiTheme'; import RootRef from '../RootRef'; -const TooltipNaked = unwrap(Tooltip); const theme = createMuiTheme(); describe('', () => { @@ -181,7 +180,7 @@ describe('', () => { describe('prop: delay', () => { it('should take the enterDelay into account', () => { - const wrapper = mount(); + const wrapper = mount(); const children = wrapper.childAt(0).childAt(0); children.simulate('focus'); assert.strictEqual(wrapper.find(Popper).props().open, false); @@ -191,7 +190,7 @@ describe('', () => { }); it('should take the leaveDelay into account', () => { - const wrapper = mount(); + const wrapper = mount(); const children = wrapper.childAt(0).childAt(0); children.simulate('focus'); clock.tick(0); @@ -286,7 +285,7 @@ describe('', () => { , ); - const tooltipNaked = wrapper.find(TooltipNaked); + const children = wrapper.childAt(0).childAt(0); children.simulate('mouseOver', { type: 'mouseOver' }); clock.tick(0); From ba36c8b3cc530a3c32e603f4e0ab97e8e37f373e Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Tue, 9 Apr 2019 22:54:11 +0100 Subject: [PATCH 03/11] Remove ref function --- packages/material-ui/src/Tooltip/Tooltip.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index fc8ca342c9ce93..198eaccc85d615 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -145,10 +145,6 @@ function Tooltip(props) { }; }); - const onRootRef = ref => { - childrenRef.current = ref; - }; - const handleOpen = event => { // The mouseover event will trigger for every nested element in the tooltip. // We can skip rerendering when the tooltip is already open. @@ -329,7 +325,7 @@ function Tooltip(props) { return ( - {React.cloneElement(children, childrenProps)} + {React.cloneElement(children, childrenProps)} Date: Tue, 9 Apr 2019 23:55:23 +0100 Subject: [PATCH 04/11] I think I've done this right --- packages/material-ui/src/Tooltip/Tooltip.js | 30 +++++++++++++++---- .../material-ui/src/Tooltip/Tooltip.test.js | 28 ++++++++--------- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index 198eaccc85d615..269a8a66ee570b 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -1,13 +1,14 @@ import React from 'react'; +import ReactDOM from 'react-dom'; import PropTypes from 'prop-types'; import warning from 'warning'; import clsx from 'clsx'; -import RootRef from '../RootRef'; import { fade } from '../styles/colorManipulator'; import withStyles from '../styles/withStyles'; import { capitalize } from '../utils/helpers'; import Grow from '../Grow'; import Popper from '../Popper'; +import { useForkRef } from '../utils/reactHelpers'; export const styles = theme => ({ /* Styles applied to the Popper component. */ @@ -72,6 +73,18 @@ export const styles = theme => ({ }, }); +function useMountedRef() { + const mountedRef = React.useRef(false); + React.useEffect(() => { + mountedRef.current = true; + return () => { + mountedRef.current = false; + }; + }, []); + + return mountedRef; +} + function Tooltip(props) { const { children, @@ -100,8 +113,13 @@ function Tooltip(props) { const ignoreNonTouchEvents = React.useRef(false); const { current: isControlled } = React.useRef(props.open != null); const childrenRef = React.useRef(); + // can be removed once we drop support for non ref forwarding class components + const handleOwnRef = React.useCallback(ref => { + childrenRef.current = ReactDOM.findDOMNode(ref); + }, []); + const handleRef = useForkRef(children.ref, handleOwnRef); const defaultId = React.useRef(); - const isMounted = React.useRef(false); + const mountedRef = useMountedRef(); const closeTimer = React.useRef(); const enterTimer = React.useRef(); const leaveTimer = React.useRef(); @@ -130,11 +148,11 @@ function Tooltip(props) { defaultId.current = `mui-tooltip-${Math.round(Math.random() * 1e5)}`; // Rerender with defaultId and childrenRef. - if (openProp && !isMounted.current) { + if (openProp && !mountedRef.current) { forceUpdate(); } - isMounted.current = true; - }, [isControlled, title, openProp]); + mountedRef.current = true; + }, [isControlled, title, openProp, mountedRef]); React.useEffect(() => { return () => { @@ -325,7 +343,7 @@ function Tooltip(props) { return ( - {React.cloneElement(children, childrenProps)} + {React.cloneElement(children, { ref: handleRef, ...childrenProps })} ', () => { let classes; let clock; const defaultProps = { - children: Hello World, + children: Hello World, theme, title: 'Hello World', }; @@ -36,7 +35,6 @@ describe('', () => { it('should render the correct structure', () => { const wrapper = mount(); const children = wrapper.childAt(0); - assert.strictEqual(children.childAt(0).type(), RootRef); assert.strictEqual(children.childAt(1).type(), Popper); assert.strictEqual(children.childAt(1).hasClass(classes.popper), true); }); @@ -86,7 +84,7 @@ describe('', () => { it('should respond to external events', () => { const wrapper = mount(); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); assert.strictEqual(wrapper.find(Popper).props().open, false); children.simulate('mouseOver'); assert.strictEqual(wrapper.find(Popper).props().open, true); @@ -101,7 +99,7 @@ describe('', () => { const wrapper = mount( , ); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); assert.strictEqual(handleRequestOpen.callCount, 0); assert.strictEqual(handleClose.callCount, 0); children.simulate('mouseOver'); @@ -114,7 +112,7 @@ describe('', () => { it('should close when the interaction is over', () => { const wrapper = mount(); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); assert.strictEqual(wrapper.find(Popper).props().open, false); children.simulate('mouseOver'); children.simulate('focus'); @@ -128,7 +126,7 @@ describe('', () => { describe('touch screen', () => { it('should not respond to quick events', () => { const wrapper = mount(); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); children.simulate('touchStart'); children.simulate('touchEnd'); children.simulate('focus'); @@ -137,7 +135,7 @@ describe('', () => { it('should open on long press', () => { const wrapper = mount(); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); children.simulate('touchStart'); children.simulate('focus'); clock.tick(1000); @@ -181,7 +179,7 @@ describe('', () => { describe('prop: delay', () => { it('should take the enterDelay into account', () => { const wrapper = mount(); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); children.simulate('focus'); assert.strictEqual(wrapper.find(Popper).props().open, false); clock.tick(111); @@ -191,7 +189,7 @@ describe('', () => { it('should take the leaveDelay into account', () => { const wrapper = mount(); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); children.simulate('focus'); clock.tick(0); assert.strictEqual(wrapper.find(Popper).props().open, true); @@ -217,12 +215,12 @@ describe('', () => { const handler = spy(); const wrapper = mount( - , ); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); const type = name.slice(2).toLowerCase(); children.simulate(type); clock.tick(0); @@ -282,11 +280,13 @@ describe('', () => { it('should keep the overlay open if the popper element is hovered', () => { const wrapper = mount( - + , ); - const children = wrapper.childAt(0).childAt(0); + const children = wrapper.find('#testChild'); children.simulate('mouseOver', { type: 'mouseOver' }); clock.tick(0); assert.strictEqual(wrapper.find(Popper).props().open, true); From 52f8ff675312097f409eb445d85e3a7c699a7efa Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Wed, 10 Apr 2019 00:15:03 +0100 Subject: [PATCH 05/11] Add forceUpdate on children mount --- packages/material-ui/src/Tooltip/Tooltip.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index 269a8a66ee570b..2db5c5d419f7e3 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -110,12 +110,15 @@ function Tooltip(props) { ...other } = props; + const [openState, setOpenState] = React.useState(false); + const [, forceUpdate] = React.useState(0); const ignoreNonTouchEvents = React.useRef(false); const { current: isControlled } = React.useRef(props.open != null); const childrenRef = React.useRef(); // can be removed once we drop support for non ref forwarding class components const handleOwnRef = React.useCallback(ref => { childrenRef.current = ReactDOM.findDOMNode(ref); + forceUpdate(n => !n); }, []); const handleRef = useForkRef(children.ref, handleOwnRef); const defaultId = React.useRef(); @@ -124,8 +127,6 @@ function Tooltip(props) { const enterTimer = React.useRef(); const leaveTimer = React.useRef(); const touchTimer = React.useRef(); - const [, forceUpdate] = React.useState(); - const [openState, setOpenState] = React.useState(false); React.useEffect(() => { warning( @@ -149,9 +150,8 @@ function Tooltip(props) { // Rerender with defaultId and childrenRef. if (openProp && !mountedRef.current) { - forceUpdate(); + forceUpdate(n => !n); } - mountedRef.current = true; }, [isControlled, title, openProp, mountedRef]); React.useEffect(() => { From 4fcfd1379ed574fae0c2acba3901d784f145c91a Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Thu, 11 Apr 2019 00:51:03 +0100 Subject: [PATCH 06/11] Code review --- packages/material-ui/src/Tooltip/Tooltip.js | 55 ++++++++++--------- .../material-ui/src/Tooltip/Tooltip.test.js | 40 +++++--------- 2 files changed, 45 insertions(+), 50 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index 2db5c5d419f7e3..0d9f6870c66aba 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -114,13 +114,12 @@ function Tooltip(props) { const [, forceUpdate] = React.useState(0); const ignoreNonTouchEvents = React.useRef(false); const { current: isControlled } = React.useRef(props.open != null); - const childrenRef = React.useRef(); + const [childNode, setChildNode] = React.useState(); // can be removed once we drop support for non ref forwarding class components const handleOwnRef = React.useCallback(ref => { - childrenRef.current = ReactDOM.findDOMNode(ref); - forceUpdate(n => !n); + setChildNode(ReactDOM.findDOMNode(ref)); }, []); - const handleRef = useForkRef(children.ref, handleOwnRef); + const handleRef = useForkRef(childNode, handleOwnRef); const defaultId = React.useRef(); const mountedRef = useMountedRef(); const closeTimer = React.useRef(); @@ -129,30 +128,34 @@ function Tooltip(props) { const touchTimer = React.useRef(); React.useEffect(() => { - warning( - !childrenRef.current.disabled || - (childrenRef.current.disabled && isControlled) || - (childrenRef.current.disabled && title === '') || - childrenRef.current.tagName.toLowerCase() !== 'button', - [ - 'Material-UI: you are providing a disabled `button` child to the Tooltip component.', - 'A disabled element does not fire events.', - "Tooltip needs to listen to the child element's events to display the title.", - '', - 'Place a `div` container on top of the element.', - ].join('\n'), - ); + if (childNode) { + warning( + !childNode.disabled || + (childNode.disabled && isControlled) || + (childNode.disabled && title === '') || + childNode.tagName.toLowerCase() !== 'button', + [ + 'Material-UI: you are providing a disabled `button` child to the Tooltip component.', + 'A disabled element does not fire events.', + "Tooltip needs to listen to the child element's events to display the title.", + '', + 'Place a `div` container on top of the element.', + ].join('\n'), + ); + } + }, [isControlled, title, childNode]); + React.useEffect(() => { // Fallback to this default id when possible. // Use the random value for client side rendering only. // We can't use it server-side. defaultId.current = `mui-tooltip-${Math.round(Math.random() * 1e5)}`; - // Rerender with defaultId and childrenRef. + // Rerender with defaultId and childNode. if (openProp && !mountedRef.current) { forceUpdate(n => !n); } - }, [isControlled, title, openProp, mountedRef]); + }, [mountedRef, openProp]); React.useEffect(() => { return () => { @@ -161,7 +164,7 @@ function Tooltip(props) { clearTimeout(leaveTimer.current); clearTimeout(touchTimer.current); }; - }); + }, []); const handleOpen = event => { // The mouseover event will trigger for every nested element in the tooltip. @@ -190,7 +193,9 @@ function Tooltip(props) { // Remove the title ahead of time. // We don't want to wait for the next render commit. // We would risk displaying two tooltips at the same time (native + this one). - childrenRef.current.setAttribute('title', ''); + if (childNode) { + childNode.setAttribute('title', ''); + } clearTimeout(enterTimer.current); clearTimeout(leaveTimer.current); @@ -208,8 +213,8 @@ function Tooltip(props) { // Workaround for https://github.com/facebook/react/issues/7769 // The autoFocus of React might trigger the event before the componentDidMount. // We need to account for this eventuality. - if (!childrenRef.current) { - childrenRef.current = event.currentTarget; + if (!childNode) { + setChildNode(event.currentTarget); } handleEnter(event); @@ -349,8 +354,8 @@ function Tooltip(props) { [classes.popperInteractive]: interactive, })} placement={placement} - anchorEl={childrenRef.current} - open={childrenRef.current ? open : false} + anchorEl={childNode} + open={childNode ? open : false} id={childrenProps['aria-describedby']} transition {...interactiveWrapperListeners} diff --git a/packages/material-ui/src/Tooltip/Tooltip.test.js b/packages/material-ui/src/Tooltip/Tooltip.test.js index 355368d5e80fd5..a69afb408fd2f8 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.test.js +++ b/packages/material-ui/src/Tooltip/Tooltip.test.js @@ -55,12 +55,12 @@ describe('', () => { describe('prop: title', () => { it('should display if the title is present', () => { const wrapper = mount(); - assert.strictEqual(wrapper.find(Popper).props().open, true); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), true); }); it('should not display if the title is an empty string', () => { const wrapper = mount(); - assert.strictEqual(wrapper.find(Popper).props().open, false); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), false); }); it('should be passed down to the child as a native title', () => { @@ -85,11 +85,14 @@ describe('', () => { it('should respond to external events', () => { const wrapper = mount(); const children = wrapper.find('#testChild'); - assert.strictEqual(wrapper.find(Popper).props().open, false); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), false); children.simulate('mouseOver'); - assert.strictEqual(wrapper.find(Popper).props().open, true); + children.simulate('focus'); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), true); children.simulate('mouseLeave'); assert.strictEqual(wrapper.find(Popper).props().open, false); + children.simulate('blur'); + assert.strictEqual(wrapper.find(Popper).props().open, false); }); it('should be controllable', () => { @@ -110,19 +113,6 @@ describe('', () => { assert.strictEqual(handleClose.callCount, 1); }); - it('should close when the interaction is over', () => { - const wrapper = mount(); - const children = wrapper.find('#testChild'); - assert.strictEqual(wrapper.find(Popper).props().open, false); - children.simulate('mouseOver'); - children.simulate('focus'); - assert.strictEqual(wrapper.find(Popper).props().open, true); - children.simulate('mouseLeave'); - assert.strictEqual(wrapper.find(Popper).props().open, false); - children.simulate('blur'); - assert.strictEqual(wrapper.find(Popper).props().open, false); - }); - describe('touch screen', () => { it('should not respond to quick events', () => { const wrapper = mount(); @@ -130,7 +120,7 @@ describe('', () => { children.simulate('touchStart'); children.simulate('touchEnd'); children.simulate('focus'); - assert.strictEqual(wrapper.find(Popper).props().open, false); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), false); }); it('should open on long press', () => { @@ -140,7 +130,7 @@ describe('', () => { children.simulate('focus'); clock.tick(1000); wrapper.update(); - assert.strictEqual(wrapper.find(Popper).props().open, true); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), true); children.simulate('touchEnd'); children.simulate('blur'); clock.tick(1500); @@ -169,10 +159,10 @@ describe('', () => { const wrapper = mount(); wrapper.setProps({ open: true }); - assert.strictEqual(wrapper.find(Popper).props().open, false); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), false); clock.tick(0); wrapper.update(); - assert.strictEqual(wrapper.find(Popper).props().open, true); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), true); }); }); @@ -181,10 +171,10 @@ describe('', () => { const wrapper = mount(); const children = wrapper.find('#testChild'); children.simulate('focus'); - assert.strictEqual(wrapper.find(Popper).props().open, false); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), false); clock.tick(111); wrapper.update(); - assert.strictEqual(wrapper.find(Popper).props().open, true); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), true); }); it('should take the leaveDelay into account', () => { @@ -192,9 +182,9 @@ describe('', () => { const children = wrapper.find('#testChild'); children.simulate('focus'); clock.tick(0); - assert.strictEqual(wrapper.find(Popper).props().open, true); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), true); children.simulate('blur'); - assert.strictEqual(wrapper.find(Popper).props().open, true); + assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), true); clock.tick(111); wrapper.update(); assert.strictEqual(wrapper.find(Popper).props().open, false); From bd664828f4560b9ee5887212e177eb2ad654d96e Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Fri, 12 Apr 2019 18:50:59 +0100 Subject: [PATCH 07/11] Code review --- packages/material-ui/src/Tooltip/Tooltip.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index 0d9f6870c66aba..e4feddf56ac789 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -149,13 +149,15 @@ function Tooltip(props) { // Fallback to this default id when possible. // Use the random value for client side rendering only. // We can't use it server-side. - defaultId.current = `mui-tooltip-${Math.round(Math.random() * 1e5)}`; + if (!defaultId.current) { + defaultId.current = `mui-tooltip-${Math.round(Math.random() * 1e5)}`; + } // Rerender with defaultId and childNode. - if (openProp && !mountedRef.current) { + if (openProp) { forceUpdate(n => !n); } - }, [mountedRef, openProp]); + }, [openProp]); React.useEffect(() => { return () => { From 345458b7e2d7390e2872158405bc05607237fcb6 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Fri, 12 Apr 2019 19:08:23 +0100 Subject: [PATCH 08/11] Remove mountRef --- packages/material-ui/src/Tooltip/Tooltip.js | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index e4feddf56ac789..cd2617cb70844b 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -73,18 +73,6 @@ export const styles = theme => ({ }, }); -function useMountedRef() { - const mountedRef = React.useRef(false); - React.useEffect(() => { - mountedRef.current = true; - return () => { - mountedRef.current = false; - }; - }, []); - - return mountedRef; -} - function Tooltip(props) { const { children, @@ -121,7 +109,6 @@ function Tooltip(props) { }, []); const handleRef = useForkRef(childNode, handleOwnRef); const defaultId = React.useRef(); - const mountedRef = useMountedRef(); const closeTimer = React.useRef(); const enterTimer = React.useRef(); const leaveTimer = React.useRef(); @@ -131,7 +118,7 @@ function Tooltip(props) { if (childNode) { warning( !childNode.disabled || - (childNode.disabled && isControlled) || + (childNode.disabled && isControlled) || (childNode.disabled && title === '') || childNode.tagName.toLowerCase() !== 'button', [ From 66268311d5e65ad85ca203117ad60a792e147621 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 14 Apr 2019 01:30:24 +0100 Subject: [PATCH 09/11] Fix mistake --- packages/material-ui/src/Tooltip/Tooltip.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index cd2617cb70844b..f331fa1b594747 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -107,7 +107,7 @@ function Tooltip(props) { const handleOwnRef = React.useCallback(ref => { setChildNode(ReactDOM.findDOMNode(ref)); }, []); - const handleRef = useForkRef(childNode, handleOwnRef); + const handleRef = useForkRef(children.ref, handleOwnRef); const defaultId = React.useRef(); const closeTimer = React.useRef(); const enterTimer = React.useRef(); From f74dcbfa7ffcb4181209b59317a700f56cc404a8 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 14 Apr 2019 01:51:24 +0100 Subject: [PATCH 10/11] Re-order --- packages/material-ui/src/Tooltip/Tooltip.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index f331fa1b594747..4619e3aa78ea41 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -100,19 +100,19 @@ function Tooltip(props) { const [openState, setOpenState] = React.useState(false); const [, forceUpdate] = React.useState(0); + const [childNode, setChildNode] = React.useState(); const ignoreNonTouchEvents = React.useRef(false); const { current: isControlled } = React.useRef(props.open != null); - const [childNode, setChildNode] = React.useState(); - // can be removed once we drop support for non ref forwarding class components - const handleOwnRef = React.useCallback(ref => { - setChildNode(ReactDOM.findDOMNode(ref)); - }, []); - const handleRef = useForkRef(children.ref, handleOwnRef); const defaultId = React.useRef(); const closeTimer = React.useRef(); const enterTimer = React.useRef(); const leaveTimer = React.useRef(); const touchTimer = React.useRef(); + // can be removed once we drop support for non ref forwarding class components + const handleOwnRef = React.useCallback(ref => { + setChildNode(ReactDOM.findDOMNode(ref)); + }, []); + const handleRef = useForkRef(children.ref, handleOwnRef); React.useEffect(() => { if (childNode) { From 541fc2fc8567092a16dcaa6e033c57bc7532dbdb Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 14 Apr 2019 14:10:09 +0100 Subject: [PATCH 11/11] Boolean fun --- packages/material-ui/src/Tooltip/Tooltip.js | 31 +++++++++++---------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js index 4619e3aa78ea41..536aa3db488b77 100644 --- a/packages/material-ui/src/Tooltip/Tooltip.js +++ b/packages/material-ui/src/Tooltip/Tooltip.js @@ -115,21 +115,22 @@ function Tooltip(props) { const handleRef = useForkRef(children.ref, handleOwnRef); React.useEffect(() => { - if (childNode) { - warning( - !childNode.disabled || - (childNode.disabled && isControlled) || - (childNode.disabled && title === '') || - childNode.tagName.toLowerCase() !== 'button', - [ - 'Material-UI: you are providing a disabled `button` child to the Tooltip component.', - 'A disabled element does not fire events.', - "Tooltip needs to listen to the child element's events to display the title.", - '', - 'Place a `div` container on top of the element.', - ].join('\n'), - ); - } + warning( + !( + childNode && + childNode.disabled && + !isControlled && + title !== '' && + childNode.tagName.toLowerCase() === 'button' + ), + [ + 'Material-UI: you are providing a disabled `button` child to the Tooltip component.', + 'A disabled element does not fire events.', + "Tooltip needs to listen to the child element's events to display the title.", + '', + 'Place a `div` container on top of the element.', + ].join('\n'), + ); }, [isControlled, title, childNode]); React.useEffect(() => {