Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Forward ref in Collapse, Popper and SwipeableDrawer #15170

Merged
merged 5 commits into from
Apr 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/material-ui/src/Collapse/Collapse.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Transition from 'react-transition-group/Transition';
import withStyles from '../styles/withStyles';
import { duration } from '../styles/transitions';
import { getTransitionProps } from '../transitions/utils';
import withForwardedRef from '../utils/withForwardedRef';

export const styles = theme => ({
/* Styles applied to the container element. */
Expand Down Expand Up @@ -132,6 +133,7 @@ class Collapse extends React.Component {
collapsedHeight,
component: Component,
in: inProp,
innerRef,
onEnter,
onEntered,
onEntering,
Expand Down Expand Up @@ -169,6 +171,7 @@ class Collapse extends React.Component {
minHeight: collapsedHeight,
...style,
}}
ref={innerRef}
{...childProps}
>
<div
Expand Down Expand Up @@ -213,6 +216,11 @@ Collapse.propTypes = {
* If `true`, the component will transition in.
*/
in: PropTypes.bool,
/**
* @ignore
* from `withForwardedRef`
*/
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
/**
* @ignore
*/
Expand Down Expand Up @@ -265,4 +273,4 @@ Collapse.muiSupportAuto = true;
export default withStyles(styles, {
withTheme: true,
name: 'MuiCollapse',
})(Collapse);
})(withForwardedRef(Collapse));
61 changes: 38 additions & 23 deletions packages/material-ui/src/Collapse/Collapse.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
createMount,
describeConformance,
getClasses,
unwrap,
} from '@material-ui/core/test-utils';
import Collapse from './Collapse';

Expand All @@ -34,13 +33,18 @@ describe('<Collapse />', () => {
classes,
inheritComponent: 'Transition',
mount,
skip: ['refForwarding'],
testComponentPropWith: false,
refInstanceof: window.HTMLDivElement,
testComponentPropWith: 'span',
}));

it('should render a container around the wrapper', () => {
const wrapper = shallow(<Collapse {...props} classes={{ container: 'woofCollapse1' }} />);
const child = new ReactWrapper(wrapper.props().children('entered'));
const child = new ReactWrapper(
wrapper
.dive()
.props()
.children('entered'),
);
assert.strictEqual(child.name(), 'div');
assert.strictEqual(child.hasClass(classes.container), true);
assert.strictEqual(child.hasClass('woofCollapse1'), true);
Expand All @@ -49,7 +53,12 @@ describe('<Collapse />', () => {
it('should render a wrapper around the children', () => {
const children = <h1>Hello</h1>;
const wrapper = shallow(<Collapse {...props}>{children}</Collapse>);
const child = new ReactWrapper(wrapper.props().children('entered'));
const child = new ReactWrapper(
wrapper
.dive()
.props()
.children('entered'),
);
assert.strictEqual(child.childAt(0).name(), 'div');
assert.strictEqual(
child
Expand Down Expand Up @@ -97,7 +106,7 @@ describe('<Collapse />', () => {
}}
/>,
);
instance = wrapper.instance();
instance = wrapper.dive().instance();
element = { getBoundingClientRect: () => ({}), style: {} };
});

Expand All @@ -118,7 +127,7 @@ describe('<Collapse />', () => {

before(() => {
wrapper = shallow(<Collapse {...props} />);
instance = wrapper.instance();
instance = wrapper.dive().instance();
});

describe('handleEnter()', () => {
Expand Down Expand Up @@ -151,7 +160,7 @@ describe('<Collapse />', () => {
it('should call handleEntering', () => {
const onEnteringStub = spy();
wrapper.setProps({ onEntering: onEnteringStub });
instance = wrapper.instance();
instance = wrapper.dive().instance();
instance.handleEntering(element);

assert.strictEqual(onEnteringStub.callCount, 1);
Expand All @@ -168,7 +177,7 @@ describe('<Collapse />', () => {
restore = theme.transitions.getAutoHeightDuration;
theme.transitions.getAutoHeightDuration = stub().returns('woofCollapseStub');
wrapper.setProps({ timeout: 'auto' });
instance = wrapper.instance();
instance = wrapper.dive().instance();
});

after(() => {
Expand Down Expand Up @@ -197,7 +206,7 @@ describe('<Collapse />', () => {
it('number should set timeout to ms', () => {
timeoutMock = 3;
wrapper.setProps({ timeout: timeoutMock });
instance = wrapper.instance();
instance = wrapper.dive().instance();
instance.handleEntering(element);

assert.strictEqual(element.style.transitionDuration, `${timeoutMock}ms`);
Expand All @@ -206,7 +215,7 @@ describe('<Collapse />', () => {
it('nothing should not set timeout', () => {
const elementBackup = element;
wrapper.setProps({ timeout: undefined });
instance = wrapper.instance();
instance = wrapper.dive().instance();
instance.handleEntering(element);

assert.strictEqual(
Expand All @@ -227,7 +236,7 @@ describe('<Collapse />', () => {
handleEnteredWrapper = shallow(<Collapse {...props} />);
onEnteredSpy = spy();
handleEnteredWrapper.setProps({ onEntered: onEnteredSpy });
handleEnteredInstance = handleEnteredWrapper.instance();
handleEnteredInstance = handleEnteredWrapper.dive().instance();
element = { style: { height: 666, transitionDuration: '500ms' } };
handleEnteredInstance.handleEntered(element);
});
Expand Down Expand Up @@ -265,7 +274,7 @@ describe('<Collapse />', () => {
it('should call onExiting', () => {
const onExitingStub = spy();
wrapper.setProps({ onExiting: onExitingStub });
instance = wrapper.instance();
instance = wrapper.dive().instance();
instance.handleExiting(element);

assert.strictEqual(onExitingStub.callCount, 1);
Expand All @@ -282,7 +291,7 @@ describe('<Collapse />', () => {
restore = theme.transitions.getAutoHeightDuration;
theme.transitions.getAutoHeightDuration = stub().returns('woofCollapseStub2');
wrapper.setProps({ timeout: 'auto' });
instance = wrapper.instance();
instance = wrapper.dive().instance();
});

after(() => {
Expand Down Expand Up @@ -311,7 +320,7 @@ describe('<Collapse />', () => {
it('number should set timeout to ms', () => {
timeoutMock = 3;
wrapper.setProps({ timeout: timeoutMock });
instance = wrapper.instance();
instance = wrapper.dive().instance();
instance.handleExiting(element);

assert.strictEqual(element.style.transitionDuration, `${timeoutMock}ms`);
Expand All @@ -320,7 +329,7 @@ describe('<Collapse />', () => {
it('nothing should not set timeout', () => {
const elementBackup = element;
wrapper.setProps({ timeout: undefined });
instance = wrapper.instance();
instance = wrapper.dive().instance();
instance.handleExiting(element);

assert.strictEqual(
Expand All @@ -346,8 +355,8 @@ describe('<Collapse />', () => {

it('should return autoTransitionDuration when timeout is auto', () => {
const wrapper = shallow(<Collapse {...props} timeout="auto" />);
assert.strictEqual(wrapper.props().timeout, null);
instance = wrapper.instance();
assert.strictEqual(wrapper.dive().props().timeout, null);
instance = wrapper.dive().instance();
const next = spy();

const autoTransitionDuration = 10;
Expand All @@ -368,7 +377,7 @@ describe('<Collapse />', () => {
const timeout = 10;
const wrapper = shallow(<Collapse {...props} timeout={timeout} />);
assert.strictEqual(wrapper.props().timeout, timeout);
instance = wrapper.instance();
instance = wrapper.dive().instance();

const next = spy();
instance.addEndListener(null, next);
Expand All @@ -382,8 +391,9 @@ describe('<Collapse />', () => {
let mountInstance;

before(() => {
const CollapseNaked = unwrap(Collapse);
mountInstance = mount(<CollapseNaked classes={{}} theme={{}} />).instance();
mountInstance = mount(<Collapse />)
.find('Collapse')
.instance();
});

it('instance should have a wrapper property', () => {
Expand All @@ -396,13 +406,18 @@ describe('<Collapse />', () => {

it('should work when closed', () => {
const wrapper = shallow(<Collapse {...props} collapsedHeight={collapsedHeight} />);
const child = new ReactWrapper(wrapper.props().children('entered'));
const child = new ReactWrapper(
wrapper
.dive()
.props()
.children('entered'),
);
assert.strictEqual(child.props().style.minHeight, collapsedHeight);
});

it('should be taken into account in handleExiting', () => {
const wrapper = shallow(<Collapse {...props} collapsedHeight={collapsedHeight} />);
const instance = wrapper.instance();
const instance = wrapper.dive().instance();
const element = { style: { height: 666, transitionDuration: undefined } };
instance.handleExiting(element);
assert.strictEqual(element.style.height, collapsedHeight, 'should have 0px height');
Expand Down
20 changes: 16 additions & 4 deletions packages/material-ui/src/Popper/Popper.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
import PopperJS from 'popper.js';
import { chainPropTypes } from '@material-ui/utils';
import Portal from '../Portal';
import { setRef, withForwardedRef } from '../utils';

function flipPlacement(placement) {
const direction = (typeof window !== 'undefined' && document.body.getAttribute('dir')) || 'ltr';
Expand Down Expand Up @@ -33,7 +34,7 @@ function getAnchorEl(anchorEl) {
* Poppers rely on the 3rd party library [Popper.js](https://github.com/FezVrasta/popper.js) for positioning.
*/
class Popper extends React.Component {
tooltip = React.createRef();
tooltipRef = React.createRef();

constructor(props) {
super();
Expand Down Expand Up @@ -84,7 +85,7 @@ class Popper extends React.Component {

handleOpen = () => {
const { anchorEl, modifiers, open, placement, popperOptions = {}, disablePortal } = this.props;
const popperNode = this.tooltip.current;
const popperNode = this.tooltipRef.current;

if (!popperNode || !anchorEl || !open) {
return;
Expand Down Expand Up @@ -139,12 +140,18 @@ class Popper extends React.Component {
this.popper = null;
};

handleRef = ref => {
setRef(this.props.innerRef, ref);
setRef(this.tooltipRef, ref);
};

render() {
const {
anchorEl,
children,
container,
disablePortal,
innerRef,
keepMounted,
modifiers,
open,
Expand Down Expand Up @@ -173,7 +180,7 @@ class Popper extends React.Component {
return (
<Portal onRendered={this.handleOpen} disablePortal={disablePortal} container={container}>
<div
ref={this.tooltip}
ref={this.handleRef}
role="tooltip"
style={{
// Prevents scroll issue, waiting for Popper.js to add this style once initiated.
Expand Down Expand Up @@ -244,6 +251,11 @@ Popper.propTypes = {
* The children stay within it's parent DOM hierarchy.
*/
disablePortal: PropTypes.bool,
/**
* @ignore
* from `withForwardedRef`
*/
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
/**
* Always keep the children in the DOM.
* This property can be useful in SEO situation or
Expand Down Expand Up @@ -297,4 +309,4 @@ Popper.defaultProps = {
transition: false,
};

export default Popper;
export default withForwardedRef(Popper);
14 changes: 7 additions & 7 deletions packages/material-ui/src/Popper/Popper.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('<Popper />', () => {
classes: {},
inheritComponent: 'div',
mount,
refInstanceof: React.Component,
refInstanceof: window.HTMLDivElement,
testComponentPropWith: false,
}));

Expand All @@ -51,7 +51,7 @@ describe('<Popper />', () => {
return null;
}}
</Popper>,
);
).dive();
assert.strictEqual(renderSpy.callCount, 1);
assert.strictEqual(renderSpy.args[0][0], 'top');
});
Expand Down Expand Up @@ -87,7 +87,7 @@ describe('<Popper />', () => {
return null;
}}
</Popper>,
);
).dive();
assert.strictEqual(renderSpy.callCount, 1);
assert.strictEqual(renderSpy.args[0][0], test.out);
});
Expand All @@ -108,15 +108,15 @@ describe('<Popper />', () => {

it('should position the popper when opening', () => {
const wrapper = mount(<Popper {...defaultProps} open={false} />);
const instance = wrapper.instance();
const instance = wrapper.find('Popper').instance();
assert.strictEqual(instance.popper == null, true);
wrapper.setProps({ open: true });
assert.strictEqual(instance.popper !== null, true);
});

it('should not position the popper when closing', () => {
const wrapper = mount(<Popper {...defaultProps} open />);
const instance = wrapper.instance();
const instance = wrapper.find('Popper').instance();
assert.strictEqual(instance.popper !== null, true);
wrapper.setProps({ open: false });
assert.strictEqual(instance.popper, null);
Expand All @@ -134,7 +134,7 @@ describe('<Popper />', () => {
)}
</Popper>,
);
const instance = wrapper.instance();
const instance = wrapper.find('Popper').instance();
assert.strictEqual(wrapper.find('span').length, 1);
assert.strictEqual(wrapper.find('span').text(), 'Hello World');
assert.strictEqual(instance.popper !== null, true);
Expand Down Expand Up @@ -165,7 +165,7 @@ describe('<Popper />', () => {
.find(Grow)
.props()
.onExited();
assert.strictEqual(wrapper.state().exited, true);
assert.strictEqual(wrapper.find('Popper').instance().state.exited, true);
});
});

Expand Down
Loading