From 7d4936293cd6cf3e71c1c50135ddc4fb30f414b2 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 14 Apr 2019 03:53:38 +0100 Subject: [PATCH 1/9] [Slide] Convert to function component --- packages/material-ui/src/Slide/Slide.js | 225 ++++++------- packages/material-ui/src/Slide/Slide.test.js | 327 ++++++++++++------- 2 files changed, 313 insertions(+), 239 deletions(-) diff --git a/packages/material-ui/src/Slide/Slide.js b/packages/material-ui/src/Slide/Slide.js index 8dd707350e3cd1..2f5f6fb4a1d9e5 100644 --- a/packages/material-ui/src/Slide/Slide.js +++ b/packages/material-ui/src/Slide/Slide.js @@ -3,11 +3,10 @@ import React from 'react'; import PropTypes from 'prop-types'; import ReactDOM from 'react-dom'; -import EventListener from 'react-event-listener'; import debounce from 'debounce'; // < 1kb payload overhead when lodash/debounce is > 3kb. import Transition from 'react-transition-group/Transition'; import ownerWindow from '../utils/ownerWindow'; -import { setRef } from '../utils/reactHelpers'; +import { useForkRef } from '../utils/reactHelpers'; import withTheme from '../styles/withTheme'; import { duration } from '../styles/transitions'; import { reflow, getTransitionProps } from '../transitions/utils'; @@ -73,63 +72,43 @@ export function setTranslateValue(props, node) { * The Slide transition is used by the [Drawer](/demos/drawers/) component. * It uses [react-transition-group](https://github.com/reactjs/react-transition-group) internally. */ -class Slide extends React.Component { - mounted = false; - - constructor() { - super(); - - if (typeof window !== 'undefined') { - this.handleResize = debounce(() => { - // Skip configuration where the position is screen size invariant. - if (this.props.in || this.props.direction === 'down' || this.props.direction === 'right') { - return; - } - - if (this.childDOMNode) { - setTranslateValue(this.props, this.childDOMNode); - } - }, 166); // Corresponds to 10 frames at 60 Hz. - } - } - - componentDidMount() { - this.mounted = true; - - // state.mounted handle SSR, once the component is mounted, we need - // to properly hide it. - if (!this.props.in) { - // We need to set initial translate values of transition element - // otherwise component will be shown when in=false. - this.updatePosition(); - } - } - - componentDidUpdate(prevProps) { - if (prevProps.direction !== this.props.direction && !this.props.in) { - // We need to update the position of the drawer when the direction change and - // when it's hidden. - this.updatePosition(); - } - } - - componentWillUnmount() { - this.handleResize.clear(); - } +function Slide(props) { + const { + children, + direction, + in: inProp, + onEnter, + onEntering, + onExit, + onExited, + style, + theme, + ...other + } = props; + + const mountedRef = React.useRef(false); + const childrenRef = React.useRef(); + const [prevDirection, setPrevDirection] = React.useState(); + /** + * used in cloneElement(children, { ref: handleRef }) + */ + const handleOwnRef = ref => { + // #StrictMode ready + childrenRef.current = ReactDOM.findDOMNode(ref); + }; + const handleRef = useForkRef(children.ref, handleOwnRef); - handleEnter = node => { - setTranslateValue(this.props, node); + const handleEnter = node => { + setTranslateValue(props, node); reflow(node); - if (this.props.onEnter) { - this.props.onEnter(node); + if (onEnter) { + onEnter(node); } }; - handleEntering = node => { - const { theme } = this.props; - - const transitionProps = getTransitionProps(this.props, { + const handleEntering = node => { + const transitionProps = getTransitionProps(props, { mode: 'enter', }); node.style.webkitTransition = theme.transitions.create('-webkit-transform', { @@ -142,15 +121,13 @@ class Slide extends React.Component { }); node.style.webkitTransform = 'translate(0, 0)'; node.style.transform = 'translate(0, 0)'; - if (this.props.onEntering) { - this.props.onEntering(node); + if (onEntering) { + onEntering(node); } }; - handleExit = node => { - const { theme } = this.props; - - const transitionProps = getTransitionProps(this.props, { + const handleExit = node => { + const transitionProps = getTransitionProps(props, { mode: 'exit', }); node.style.webkitTransition = theme.transitions.create('-webkit-transform', { @@ -161,78 +138,92 @@ class Slide extends React.Component { ...transitionProps, easing: theme.transitions.easing.sharp, }); - setTranslateValue(this.props, node); + setTranslateValue(props, node); - if (this.props.onExit) { - this.props.onExit(node); + if (onExit) { + onExit(node); } }; - handleExited = node => { + const handleExited = node => { // No need for transitions when the component is hidden node.style.webkitTransition = ''; node.style.transition = ''; - if (this.props.onExited) { - this.props.onExited(node); + if (onExited) { + onExited(node); } }; - /** - * used in cloneElement(children, { ref: handleRef }) - */ - handleRef = ref => { - // #StrictMode ready - this.childDOMNode = ReactDOM.findDOMNode(ref); - setRef(this.props.children.ref, ref); - }; + const updatePosition = React.useCallback(() => { + if (childrenRef.current) { + setTranslateValue(props, childrenRef.current); + } + }, [props]); + + React.useEffect(() => { + const handleResize = debounce(() => { + // Skip configuration where the position is screen size invariant. + if (inProp || direction === 'down' || direction === 'right') { + return; + } + + if (childrenRef.current) { + setTranslateValue(props, childrenRef.current); + } + }, 166); // Corresponds to 10 frames at 60 Hz. + + window.addEventListener('resize', handleResize); - updatePosition() { - if (this.childDOMNode) { - setTranslateValue(this.props, this.childDOMNode); + return () => { + handleResize.clear(); + window.removeEventListener('resize', handleResize); + }; + }, [direction, inProp, props]); + + React.useEffect(() => { + // state.mounted handle SSR, once the component is mounted, we need + // to properly hide it. + if (!inProp && !mountedRef.current) { + // We need to set initial translate values of transition element + // otherwise component will be shown when in=false. + updatePosition(); } - } + mountedRef.current = true; + }, [inProp, updatePosition]); - render() { - const { - children, - direction, - in: inProp, - onEnter, - onEntering, - onExit, - onExited, - style, - theme, - ...other - } = this.props; - - return ( - - - {(state, childProps) => { - return React.cloneElement(children, { - ref: this.handleRef, - style: { - visibility: state === 'exited' && !inProp ? 'hidden' : undefined, - ...style, - ...children.props.style, - }, - ...childProps, - }); - }} - - - ); - } + React.useEffect(() => { + if (prevDirection !== direction && !inProp) { + // We need to update the position of the drawer when the direction change and + // when it's hidden. + updatePosition(); + } + setPrevDirection(direction); + }, [direction, inProp, prevDirection, updatePosition]); + + return ( + + {(state, childProps) => { + return React.cloneElement(children, { + ref: handleRef, + style: { + visibility: state === 'exited' && !inProp ? 'hidden' : undefined, + ...style, + ...children.props.style, + }, + ...childProps, + }); + }} + + ); } Slide.propTypes = { diff --git a/packages/material-ui/src/Slide/Slide.test.js b/packages/material-ui/src/Slide/Slide.test.js index 6a66e722f1df20..431d7c3238ecae 100644 --- a/packages/material-ui/src/Slide/Slide.test.js +++ b/packages/material-ui/src/Slide/Slide.test.js @@ -1,28 +1,20 @@ import React from 'react'; import { assert } from 'chai'; -import { spy, useFakeTimers } from 'sinon'; -import { - createShallow, - createMount, - describeConformance, - unwrap, -} from '@material-ui/core/test-utils'; +import { spy, stub, useFakeTimers } from 'sinon'; +import { createMount, describeConformance } from '@material-ui/core/test-utils'; import Slide, { setTranslateValue } from './Slide'; import transitions, { easing } from '../styles/transitions'; import createMuiTheme from '../styles/createMuiTheme'; describe('', () => { - let shallow; let mount; - const SlideNaked = unwrap(Slide); const defaultProps = { in: true, - children:
, + children:
, direction: 'down', }; before(() => { - shallow = createShallow({ dive: true }); mount = createMount(); }); @@ -39,19 +31,19 @@ describe('', () => { inheritComponent: 'Transition', mount, refInstanceof: React.Component, - skip: ['componentProp'], + skip: ['componentProp', 'refForwarding'], }), ); it('should not override children styles', () => { const wrapper = mount( -
- , + , ); assert.deepEqual(wrapper.find('#with-slide').props().style, { backgroundColor: 'yellow', @@ -60,192 +52,284 @@ describe('', () => { }); }); - describe('event callbacks', () => { - it('should fire event callbacks', () => { - const events = ['onEnter', 'onEntering', 'onEntered', 'onExit', 'onExiting', 'onExited']; + describe('transition lifecycle', () => { + let wrapper; + let clock; + let child; + + const handleEnter = spy(); + const handleEntering = spy(); + const handleEntered = spy(); + const handleExit = spy(); + const handleExiting = spy(); + const handleExited = spy(); + + before(() => { + wrapper = mount( + +
{ + child = ref; + }} + /> + , + ); + clock = useFakeTimers(); + }); + + after(() => { + clock.restore(); + }); + + describe('in', () => { + before(() => { + wrapper.setProps({ in: true }); + }); - const handlers = events.reduce((result, n) => { - result[n] = spy(); - return result; - }, {}); + describe('handleEnter()', () => { + it('should call handleEnter', () => { + assert.strictEqual(handleEntering.callCount, 1); + assert.strictEqual(handleEntering.args[0][0], child); + }); + }); - const wrapper = shallow().childAt(0); + describe('handleEntering()', () => { + it('should reset the translate3d', () => { + assert.strictEqual(handleEntering.args[0][0].style.transform, 'translate(0, 0)'); + }); - events.forEach(n => { - const event = n.charAt(2).toLowerCase() + n.slice(3); - wrapper.simulate(event, { - fakeTransform: 'none', - style: {}, - getBoundingClientRect: () => ({}), + it('should call handleEntering', () => { + assert.strictEqual(handleEntering.callCount, 1); + assert.strictEqual(handleEntering.args[0][0], child); + }); + }); + + describe('handleEntered()', () => { + it('should have called onEntered', () => { + clock.tick(1000); + assert.strictEqual(handleEntered.callCount, 1); + }); + }); + }); + + describe('out', () => { + before(() => { + wrapper.setProps({ in: true }); + wrapper.setProps({ in: false }); + }); + + describe('handleExit()', () => { + it('should call handleExit', () => { + assert.strictEqual(handleExiting.callCount, 1); + assert.strictEqual(handleExiting.args[0][0], child); + }); + }); + + describe('handleExiting()', () => { + it('should call onExiting', () => { + assert.strictEqual(handleExiting.callCount, 1); + assert.strictEqual(handleExiting.args[0][0], child); + }); + }); + + describe('handleExited()', () => { + it('should call onExited', () => { + clock.tick(1000); + assert.strictEqual(handleExited.callCount, 1); + assert.strictEqual(handleExited.args[0][0], child); }); - assert.strictEqual(handlers[n].callCount, 1, `should have called the ${n} handler`); }); }); }); describe('prop: timeout', () => { let wrapper; - let instance; - let element; const enterDuration = 556; const leaveDuration = 446; + const handleEntering = spy(); + const handleExit = spy(); beforeEach(() => { - wrapper = shallow( + wrapper = mount( , ); - instance = wrapper.instance(); - element = { fakeTransform: 'none', getBoundingClientRect: () => ({}), style: {} }; }); it('should create proper easeOut animation onEntering', () => { - instance.handleEntering(element); const animation = transitions.create('transform', { duration: enterDuration, easing: easing.easeOut, }); - assert.strictEqual(element.style.transition, animation); + assert.strictEqual(handleEntering.args[0][0].style.transition, animation); }); it('should create proper sharp animation onExit', () => { - instance.handleExit(element); + wrapper.setProps({ in: false }); const animation = transitions.create('transform', { duration: leaveDuration, easing: easing.sharp, }); - assert.strictEqual(element.style.transition, animation); + assert.strictEqual(handleExit.args[0][0].style.transition, animation); }); }); describe('prop: direction', () => { it('should update the position', () => { const wrapper = mount( - , + , ); - const transition = wrapper.instance().childDOMNode; + const child = wrapper.find('#testChild').instance(); - const transition1 = transition.style.transform; + const transition1 = child.style.transform; wrapper.setProps({ direction: 'right', }); - const transition2 = transition.style.transform; + const transition2 = child.style.transform; assert.notStrictEqual(transition1, transition2); }); }); - describe('transition lifecycle', () => { + describe('transform styling', () => { let wrapper; - let instance; + let child; + const handleEnter = spy(); + let nodeEnterTransformStyle; + const handleEnterWrapper = (...args) => { + handleEnter(...args); + nodeEnterTransformStyle = args[0].style.transform; + }; + const handleExiting = spy(); + let nodeExitingTransformStyle; + const handleExitingWrapper = (...args) => { + handleExiting(...args); + nodeExitingTransformStyle = args[0].style.transform; + }; before(() => { - wrapper = shallow(); - instance = wrapper.instance(); + wrapper = mount( + +
{ + child = ref; + }} + /> + , + ); + + child.fakeTransform = 'none'; + stub(child, 'getBoundingClientRect').callsFake(() => ({ + width: 500, + height: 300, + left: 300, + right: 800, + top: 200, + bottom: 500, + })); }); describe('handleEnter()', () => { - let element; - - beforeEach(() => { - element = { - fakeTransform: 'none', - getBoundingClientRect: () => ({ - width: 500, - height: 300, - left: 300, - right: 800, - top: 200, - bottom: 500, - }), - style: {}, - }; + afterEach(() => { + wrapper.setProps({ + in: false, + }); }); - it('should set element transform and transition according to the direction', () => { + it('should set element transform and transition in the `left` direction', () => { wrapper.setProps({ direction: 'left' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateX(100vw) translateX(-300px)'); + wrapper.setProps({ in: true }); + assert.strictEqual(nodeEnterTransformStyle, 'translateX(100vw) translateX(-300px)'); + }); + + it('should set element transform and transition in the `right` direction', () => { wrapper.setProps({ direction: 'right' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateX(-824px)'); + wrapper.setProps({ in: true }); + assert.strictEqual(nodeEnterTransformStyle, 'translateX(-824px)'); + }); + + it('should set element transform and transition in the `up` direction', () => { wrapper.setProps({ direction: 'up' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateY(100vh) translateY(-200px)'); + wrapper.setProps({ in: true }); + assert.strictEqual(nodeEnterTransformStyle, 'translateY(100vh) translateY(-200px)'); + }); + + it('should set element transform and transition in the `down` direction', () => { wrapper.setProps({ direction: 'down' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateY(-524px)'); + wrapper.setProps({ in: true }); + assert.strictEqual(nodeEnterTransformStyle, 'translateY(-524px)'); }); it('should reset the previous transition if needed', () => { - element.style.transform = 'translateX(-824px)'; + child.style.transform = 'translateX(-824px)'; wrapper.setProps({ direction: 'right' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateX(-824px)'); + wrapper.setProps({ in: true }); + assert.strictEqual(nodeEnterTransformStyle, 'translateX(-824px)'); }); }); - describe('handleEntering()', () => { - let element; - + describe('handleExiting()', () => { before(() => { - element = { style: {} }; - instance.handleEntering(element); + wrapper.setProps({ + in: true, + }); }); - it('should reset the translate3d', () => { - assert.strictEqual(element.style.transform, 'translate(0, 0)'); + afterEach(() => { + wrapper.setProps({ + in: true, + }); }); - }); - - describe('handleExiting()', () => { - let element; - before(() => { - element = { - fakeTransform: 'none', - getBoundingClientRect: () => ({ - width: 500, - height: 300, - left: 300, - right: 800, - top: 200, - bottom: 500, - }), - style: {}, - }; + it('should set element transform and transition in the `left` direction', () => { + wrapper.setProps({ direction: 'left' }); + wrapper.setProps({ in: false }); + assert.strictEqual(nodeExitingTransformStyle, 'translateX(100vw) translateX(-300px)'); }); - it('should set element transform and transition according to the direction', () => { - wrapper.setProps({ direction: 'left' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateX(100vw) translateX(-300px)'); + it('should set element transform and transition in the `right` direction', () => { wrapper.setProps({ direction: 'right' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateX(-824px)'); + wrapper.setProps({ in: false }); + assert.strictEqual(nodeExitingTransformStyle, 'translateX(-824px)'); + }); + + it('should set element transform and transition in the `up` direction', () => { wrapper.setProps({ direction: 'up' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateY(100vh) translateY(-200px)'); + wrapper.setProps({ in: false }); + assert.strictEqual(nodeExitingTransformStyle, 'translateY(100vh) translateY(-200px)'); + }); + + it('should set element transform and transition in the `down` direction', () => { wrapper.setProps({ direction: 'down' }); - instance.handleEnter(element); - assert.strictEqual(element.style.transform, 'translateY(-524px)'); + wrapper.setProps({ in: false }); + assert.strictEqual(nodeExitingTransformStyle, 'translateY(-524px)'); }); }); }); describe('mount', () => { it('should work when initially hidden', () => { - const wrapper = mount( - -
Foo
-
, + const childRef = React.createRef(); + mount( + +
Foo
+
, ); - const transition = wrapper.instance().childDOMNode; + const transition = childRef.current; assert.strictEqual(transition.style.visibility, 'hidden'); assert.notStrictEqual(transition.style.transform, undefined); @@ -265,16 +349,16 @@ describe('', () => { it('should recompute the correct position', () => { const wrapper = mount( - -
Foo
-
, + +
Foo
+
, ); - const instance = wrapper.instance(); - instance.handleResize(); + + window.dispatchEvent(new window.Event('resize', {})); clock.tick(166); - const transition = instance.childDOMNode; + const child = wrapper.find('#testChild').instance(); - assert.notStrictEqual(transition.style.transform, undefined); + assert.notStrictEqual(child.style.transform, undefined); }); it('should take existing transform into account', () => { @@ -300,9 +384,8 @@ describe('', () => { }); it('should do nothing when visible', () => { - const wrapper = shallow(); - const instance = wrapper.instance(); - instance.handleResize(); + mount(); + window.dispatchEvent(new window.Event('resize', {})); clock.tick(166); }); }); From 5b54b5d50dc31194f24bee5c75e26d3be72ee2c6 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 14 Apr 2019 14:20:01 +0100 Subject: [PATCH 2/9] Remove innerRef propType from Table --- packages/material-ui/src/Table/Table.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/material-ui/src/Table/Table.js b/packages/material-ui/src/Table/Table.js index 81d260355393c8..eefa6bd1f748e3 100644 --- a/packages/material-ui/src/Table/Table.js +++ b/packages/material-ui/src/Table/Table.js @@ -44,11 +44,6 @@ Table.propTypes = { * Either a string to use a DOM element or a component. */ component: PropTypes.elementType, - /** - * @ignore - * from `withForwardRef` - */ - innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]), /** * Allows TableCells to inherit padding of the Table. */ From b0df52197762bdf4ab80144604c569a84040b14d Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 14 Apr 2019 14:31:29 +0100 Subject: [PATCH 3/9] Handle change in style formatting in tests --- packages/material-ui/src/Slide/Slide.test.js | 21 +++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/packages/material-ui/src/Slide/Slide.test.js b/packages/material-ui/src/Slide/Slide.test.js index 431d7c3238ecae..a28e9ddf237e24 100644 --- a/packages/material-ui/src/Slide/Slide.test.js +++ b/packages/material-ui/src/Slide/Slide.test.js @@ -3,7 +3,6 @@ import { assert } from 'chai'; import { spy, stub, useFakeTimers } from 'sinon'; import { createMount, describeConformance } from '@material-ui/core/test-utils'; import Slide, { setTranslateValue } from './Slide'; -import transitions, { easing } from '../styles/transitions'; import createMuiTheme from '../styles/createMuiTheme'; describe('', () => { @@ -102,7 +101,7 @@ describe('', () => { describe('handleEntering()', () => { it('should reset the translate3d', () => { - assert.strictEqual(handleEntering.args[0][0].style.transform, 'translate(0, 0)'); + assert.match(handleEntering.args[0][0].style.transform, /translate\(0(px)?, 0(px)?\)/); }); it('should call handleEntering', () => { @@ -171,20 +170,18 @@ describe('', () => { }); it('should create proper easeOut animation onEntering', () => { - const animation = transitions.create('transform', { - duration: enterDuration, - easing: easing.easeOut, - }); - assert.strictEqual(handleEntering.args[0][0].style.transition, animation); + assert.match( + handleEntering.args[0][0].style.transition, + /transform 556ms cubic-bezier\(0(.0)?, 0, 0.2, 1\)( 0ms)?/, + ); }); it('should create proper sharp animation onExit', () => { wrapper.setProps({ in: false }); - const animation = transitions.create('transform', { - duration: leaveDuration, - easing: easing.sharp, - }); - assert.strictEqual(handleExit.args[0][0].style.transition, animation); + assert.match( + handleExit.args[0][0].style.transition, + /transform 446ms cubic-bezier\(0.4, 0, 0.6, 1\)( 0ms)?/, + ); }); }); From 99ea9123a1b4c44c90fec51983c57bb63753c0aa Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 14 Apr 2019 23:31:41 +0100 Subject: [PATCH 4/9] Code review --- packages/material-ui/src/Slide/Slide.js | 62 +++++++++++-------- packages/material-ui/src/transitions/utils.js | 4 +- pages/api/slide.md | 2 +- 3 files changed, 38 insertions(+), 30 deletions(-) diff --git a/packages/material-ui/src/Slide/Slide.js b/packages/material-ui/src/Slide/Slide.js index 2f5f6fb4a1d9e5..940a3d5c4cf45e 100644 --- a/packages/material-ui/src/Slide/Slide.js +++ b/packages/material-ui/src/Slide/Slide.js @@ -16,8 +16,7 @@ const GUTTER = 24; // Translate the node so he can't be seen on the screen. // Later, we gonna translate back the node to his original location // with `translate3d(0, 0, 0)`.` -function getTranslateValue(props, node) { - const { direction } = props; +function getTranslateValue(direction, node) { const rect = node.getBoundingClientRect(); let transform; @@ -59,8 +58,8 @@ function getTranslateValue(props, node) { return `translateY(-${rect.top + rect.height + GUTTER - offsetY}px)`; } -export function setTranslateValue(props, node) { - const transform = getTranslateValue(props, node); +export function setTranslateValue(direction, node) { + const transform = getTranslateValue(direction, node); if (transform) { node.style.webkitTransform = transform; @@ -83,12 +82,12 @@ function Slide(props) { onExited, style, theme, + timeout, ...other } = props; const mountedRef = React.useRef(false); const childrenRef = React.useRef(); - const [prevDirection, setPrevDirection] = React.useState(); /** * used in cloneElement(children, { ref: handleRef }) */ @@ -98,8 +97,9 @@ function Slide(props) { }; const handleRef = useForkRef(children.ref, handleOwnRef); - const handleEnter = node => { - setTranslateValue(props, node); + const handleEnter = () => { + const node = childrenRef.current; + setTranslateValue(direction, node); reflow(node); if (onEnter) { @@ -107,10 +107,14 @@ function Slide(props) { } }; - const handleEntering = node => { - const transitionProps = getTransitionProps(props, { - mode: 'enter', - }); + const handleEntering = () => { + const node = childrenRef.current; + const transitionProps = getTransitionProps( + { timeout, style }, + { + mode: 'enter', + }, + ); node.style.webkitTransition = theme.transitions.create('-webkit-transform', { ...transitionProps, easing: theme.transitions.easing.easeOut, @@ -126,10 +130,14 @@ function Slide(props) { } }; - const handleExit = node => { - const transitionProps = getTransitionProps(props, { - mode: 'exit', - }); + const handleExit = () => { + const node = childrenRef.current; + const transitionProps = getTransitionProps( + { timeout, style }, + { + mode: 'exit', + }, + ); node.style.webkitTransition = theme.transitions.create('-webkit-transform', { ...transitionProps, easing: theme.transitions.easing.sharp, @@ -138,14 +146,15 @@ function Slide(props) { ...transitionProps, easing: theme.transitions.easing.sharp, }); - setTranslateValue(props, node); + setTranslateValue(direction, node); if (onExit) { onExit(node); } }; - const handleExited = node => { + const handleExited = () => { + const node = childrenRef.current; // No need for transitions when the component is hidden node.style.webkitTransition = ''; node.style.transition = ''; @@ -157,9 +166,9 @@ function Slide(props) { const updatePosition = React.useCallback(() => { if (childrenRef.current) { - setTranslateValue(props, childrenRef.current); + setTranslateValue(direction, childrenRef.current); } - }, [props]); + }, [direction]); React.useEffect(() => { const handleResize = debounce(() => { @@ -169,7 +178,7 @@ function Slide(props) { } if (childrenRef.current) { - setTranslateValue(props, childrenRef.current); + setTranslateValue(direction, childrenRef.current); } }, 166); // Corresponds to 10 frames at 60 Hz. @@ -179,7 +188,7 @@ function Slide(props) { handleResize.clear(); window.removeEventListener('resize', handleResize); }; - }, [direction, inProp, props]); + }, [direction, inProp]); React.useEffect(() => { // state.mounted handle SSR, once the component is mounted, we need @@ -193,13 +202,12 @@ function Slide(props) { }, [inProp, updatePosition]); React.useEffect(() => { - if (prevDirection !== direction && !inProp) { + if (!inProp) { // We need to update the position of the drawer when the direction change and // when it's hidden. updatePosition(); } - setPrevDirection(direction); - }, [direction, inProp, prevDirection, updatePosition]); + }, [inProp, updatePosition]); return ( {(state, childProps) => { @@ -228,8 +237,9 @@ function Slide(props) { Slide.propTypes = { /** - * A single child content element. The component used as a child must be able - * to hold a ref. + * A single child content element. + * + * ⚠️The component used as a child [must be able to hold a ref](/guides/composition/#children). */ children: PropTypes.element, /** diff --git a/packages/material-ui/src/transitions/utils.js b/packages/material-ui/src/transitions/utils.js index c43f9da6791654..b9e290b374a918 100644 --- a/packages/material-ui/src/transitions/utils.js +++ b/packages/material-ui/src/transitions/utils.js @@ -1,8 +1,6 @@ export const reflow = node => node.scrollTop; -export function getTransitionProps(props, options) { - const { timeout, style = {} } = props; - +export function getTransitionProps({ timeout, style = {} }, options) { return { duration: style.transitionDuration || typeof timeout === 'number' ? timeout : timeout[options.mode], diff --git a/pages/api/slide.md b/pages/api/slide.md index 6ae9ce44b59685..a4235a3248b4eb 100644 --- a/pages/api/slide.md +++ b/pages/api/slide.md @@ -19,7 +19,7 @@ It uses [react-transition-group](https://github.com/reactjs/react-transition-gro | Name | Type | Default | Description | |:-----|:-----|:--------|:------------| -| children | element | | A single child content element. The component used as a child must be able to hold a ref. | +| children | element | | A single child content element.
⚠️The component used as a child [must be able to hold a ref](/guides/composition/#children). | | direction | enum: 'left' |
 'right' |
 'up' |
 'down'
| 'down' | Direction the child node will enter from. | | in | bool | | If `true`, show the component; triggers the enter or exit animation. | | timeout | union: number |
 { enter?: number, exit?: number }
| { enter: duration.enteringScreen, exit: duration.leavingScreen,} | The duration for the transition, in milliseconds. You may specify a single timeout for all transitions, or individually with an object. | From f5142909d298682f98035d0cfc1431b95b387215 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Mon, 15 Apr 2019 00:45:03 +0100 Subject: [PATCH 5/9] Code review v2 --- packages/material-ui/src/Slide/Slide.js | 46 +++++++------------ packages/material-ui/src/transitions/utils.js | 4 +- 2 files changed, 20 insertions(+), 30 deletions(-) diff --git a/packages/material-ui/src/Slide/Slide.js b/packages/material-ui/src/Slide/Slide.js index 940a3d5c4cf45e..8fc7ba149c2427 100644 --- a/packages/material-ui/src/Slide/Slide.js +++ b/packages/material-ui/src/Slide/Slide.js @@ -86,7 +86,6 @@ function Slide(props) { ...other } = props; - const mountedRef = React.useRef(false); const childrenRef = React.useRef(); /** * used in cloneElement(children, { ref: handleRef }) @@ -171,35 +170,24 @@ function Slide(props) { }, [direction]); React.useEffect(() => { - const handleResize = debounce(() => { - // Skip configuration where the position is screen size invariant. - if (inProp || direction === 'down' || direction === 'right') { - return; - } - - if (childrenRef.current) { - setTranslateValue(direction, childrenRef.current); - } - }, 166); // Corresponds to 10 frames at 60 Hz. - - window.addEventListener('resize', handleResize); - - return () => { - handleResize.clear(); - window.removeEventListener('resize', handleResize); - }; - }, [direction, inProp]); - - React.useEffect(() => { - // state.mounted handle SSR, once the component is mounted, we need - // to properly hide it. - if (!inProp && !mountedRef.current) { - // We need to set initial translate values of transition element - // otherwise component will be shown when in=false. - updatePosition(); + // Skip configuration where the position is screen size invariant. + if (!inProp && direction !== 'down' && direction !== 'right') { + const handleResize = debounce(() => { + if (childrenRef.current) { + setTranslateValue(direction, childrenRef.current); + } + }, 166); // Corresponds to 10 frames at 60 Hz. + + window.addEventListener('resize', handleResize); + + return () => { + handleResize.clear(); + window.removeEventListener('resize', handleResize); + }; } - mountedRef.current = true; - }, [inProp, updatePosition]); + + return () => {}; + }, [direction, inProp]); React.useEffect(() => { if (!inProp) { diff --git a/packages/material-ui/src/transitions/utils.js b/packages/material-ui/src/transitions/utils.js index b9e290b374a918..c43f9da6791654 100644 --- a/packages/material-ui/src/transitions/utils.js +++ b/packages/material-ui/src/transitions/utils.js @@ -1,6 +1,8 @@ export const reflow = node => node.scrollTop; -export function getTransitionProps({ timeout, style = {} }, options) { +export function getTransitionProps(props, options) { + const { timeout, style = {} } = props; + return { duration: style.transitionDuration || typeof timeout === 'number' ? timeout : timeout[options.mode], From 319b057640fa7e09a4c58956f9ca31271cb05a32 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Mon, 15 Apr 2019 00:49:42 +0100 Subject: [PATCH 6/9] Fix tests --- packages/material-ui/src/Slide/Slide.test.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/material-ui/src/Slide/Slide.test.js b/packages/material-ui/src/Slide/Slide.test.js index a28e9ddf237e24..903a304c4cbdcd 100644 --- a/packages/material-ui/src/Slide/Slide.test.js +++ b/packages/material-ui/src/Slide/Slide.test.js @@ -371,12 +371,7 @@ describe('', () => { }), style: {}, }; - setTranslateValue( - { - direction: 'up', - }, - element, - ); + setTranslateValue('up', element); assert.strictEqual(element.style.transform, 'translateY(100vh) translateY(-780px)'); }); From 887730b8b77dde491ca0bbf296f4ae8d2f7df111 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Tue, 16 Apr 2019 22:25:01 +0100 Subject: [PATCH 7/9] Add useCallback to handleOwnRef --- packages/material-ui/src/Slide/Slide.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/material-ui/src/Slide/Slide.js b/packages/material-ui/src/Slide/Slide.js index 8fc7ba149c2427..bbcdb79b4a3e3a 100644 --- a/packages/material-ui/src/Slide/Slide.js +++ b/packages/material-ui/src/Slide/Slide.js @@ -90,10 +90,10 @@ function Slide(props) { /** * used in cloneElement(children, { ref: handleRef }) */ - const handleOwnRef = ref => { + const handleOwnRef = React.useCallback(ref => { // #StrictMode ready childrenRef.current = ReactDOM.findDOMNode(ref); - }; + }, []); const handleRef = useForkRef(children.ref, handleOwnRef); const handleEnter = () => { From 893ba76753135b6a253f219211e8bfeab533f87e Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Thu, 18 Apr 2019 14:10:28 +0100 Subject: [PATCH 8/9] Prettier --- packages/material-ui/src/Slide/Slide.test.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/material-ui/src/Slide/Slide.test.js b/packages/material-ui/src/Slide/Slide.test.js index e4c4c30497465c..a6915c93fb1e24 100644 --- a/packages/material-ui/src/Slide/Slide.test.js +++ b/packages/material-ui/src/Slide/Slide.test.js @@ -188,9 +188,7 @@ describe('', () => { describe('prop: direction', () => { it('should update the position', () => { - const wrapper = mount( - , - ); + const wrapper = mount(); const child = wrapper.find('#testChild').instance(); const transition1 = child.style.transform; From 3df7d1757702de6fb630eef3c6cc2f87fed353a0 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Thu, 18 Apr 2019 15:42:52 +0100 Subject: [PATCH 9/9] Add breaking change to migration docs --- .../pages/guides/migration-v3/migration-v3.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/src/pages/guides/migration-v3/migration-v3.md b/docs/src/pages/guides/migration-v3/migration-v3.md index 232e0a0630e0c8..8040da9f2ca717 100644 --- a/docs/src/pages/guides/migration-v3/migration-v3.md +++ b/docs/src/pages/guides/migration-v3/migration-v3.md @@ -340,3 +340,20 @@ You should be able to move the custom styles to the root class key. `event.preventDefault()` is meant to stop default behaviors like clicking a checkbox to check it, hitting a button to submit a form, and hitting left arrow to move the cursor in a text input etc. Only special HTML elements have these default behaviors. People should use `event.stopPropagation()` if they don't want to trigger a `onClose` event on the modal. + +### Slide + +- [Slide] The child needs to be able to hold a ref. + + ```diff + class Component extends React.Component { + render() { + return
+ } + } + -const MyComponent = props =>
+ +const MyComponent = React.forwardRef((props, ref) =>
); + + +
+ ```