Skip to content

[Portal] Migrate to React hooks #15399

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.

Sign up for GitHub

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

Merged
oliviertassinari merged 4 commits into from
Apr 30, 2019
Merged

[Portal] Migrate to React hooks #15399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd0a4d8
portal migrated to hooks
gautam-pahuja Apr 18, 2019
6d2e364
complete the effort :)
oliviertassinari Apr 27, 2019
b7f9581
ref weak convention
oliviertassinari Apr 29, 2019
8560282
use null when ref is applied to an element
oliviertassinari Apr 30, 2019
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
2 changes: 1 addition & 1 deletion docs/src/modules/components/AppSearch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ const useStyles = makeStyles(theme => ({
function AppSearch(props) {
const { userLanguage } = props;
const classes = useStyles();
const inputRef = React.useRef();
const inputRef = React.useRef(null);
const theme = useTheme();

React.useEffect(() => {
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/demos/progress/CircularIntegration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function CircularIntegration() {
const classes = useStyles();
const [loading, setLoading] = React.useState(false);
const [success, setSuccess] = React.useState(false);
const timer = React.useRef(undefined);
const timer = React.useRef();

const buttonClassname = clsx({
[classes.buttonSuccess]: success,
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/demos/progress/CircularIntegration.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function CircularIntegration() {
const classes = useStyles();
const [loading, setLoading] = React.useState(false);
const [success, setSuccess] = React.useState(false);
const timer = React.useRef<number | undefined>(undefined);
const timer = React.useRef<number>();

const buttonClassname = clsx({
[classes.buttonSuccess]: success,
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function ClickAwayListener(props) {
const mountedRef = useMountedRef();
const movedRef = React.useRef(false);

const nodeRef = React.useRef();
const nodeRef = React.useRef(null);
// can be removed once we drop support for non ref forwarding class components
const handleOwnRef = React.useCallback(instance => {
// #StrictMode ready
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Collapse/Collapse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ const Collapse = React.forwardRef(function Collapse(props, ref) {
...other
} = props;
const timer = React.useRef();
const wrapperRef = React.useRef();
const wrapperRef = React.useRef(null);
const autoTransitionDuration = React.useRef();

React.useEffect(() => {
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Dialog/Dialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ const Dialog = React.forwardRef(function Dialog(props, ref) {
...other
} = props;

const mouseDownTarget = React.useRef(null);
const mouseDownTarget = React.useRef();
const handleMouseDown = event => {
mouseDownTarget.current = event.target;
};
Expand Down
4 changes: 2 additions & 2 deletions packages/material-ui/src/InputBase/Textarea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ const Textarea = React.forwardRef(function Textarea(props, ref) {
const { onChange, rows, rowsMax, style, value, ...other } = props;

const { current: isControlled } = React.useRef(value != null);
const inputRef = React.useRef();
const inputRef = React.useRef(null);
const [state, setState] = React.useState({});
const shadowRef = React.useRef();
const shadowRef = React.useRef(null);
const handleRef = useForkRef(ref, inputRef);

const syncHeight = React.useCallback(() => {
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/ListItem/ListItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
dense: dense || context.dense || false,
alignItems,
};
const listItemRef = React.useRef();
const listItemRef = React.useRef(null);
useEnhancedEffect(() => {
if (autoFocus) {
if (listItemRef.current) {
Expand Down
6 changes: 3 additions & 3 deletions packages/material-ui/src/Menu/Menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ const Menu = React.forwardRef(function Menu(props, ref) {

const autoFocus = autoFocusProp !== undefined ? autoFocusProp : !disableAutoFocusItem;

const menuListActionsRef = React.useRef();
const firstValidItemRef = React.useRef();
const firstSelectedItemRef = React.useRef();
const menuListActionsRef = React.useRef(null);
const firstValidItemRef = React.useRef(null);
const firstSelectedItemRef = React.useRef(null);

const getContentAnchorEl = () => firstSelectedItemRef.current || firstValidItemRef.current;

Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/MenuList/MenuList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const useEnhancedEffect = typeof window === 'undefined' ? React.useEffect : Reac

const MenuList = React.forwardRef(function MenuList(props, ref) {
const { actions, autoFocus, className, onKeyDown, disableListWrap, ...other } = props;
const listRef = React.useRef();
const listRef = React.useRef(null);
const textCriteriaRef = React.useRef({
keys: [],
repeating: true,
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Modal/Modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ class Modal extends React.Component {
};

handlePortalRef = ref => {
this.mountNode = ref ? ref.getMountNode() : ref;
this.mountNode = ref;
};

handleModalRef = ref => {
Expand Down
6 changes: 1 addition & 5 deletions packages/material-ui/src/Modal/Modal.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
describeConformance,
} from '@material-ui/core/test-utils';
import Fade from '../Fade';
import Portal from '../Portal';
import Backdrop from '../Backdrop';
import Modal from './Modal';

Expand Down Expand Up @@ -176,10 +175,7 @@ describe('<Modal />', () => {

it('should render the content into the portal', () => {
wrapper.setProps({ open: true });
const portalLayer = wrapper
.find(Portal)
.instance()
.getMountNode();
const portalLayer = document.querySelector('[data-mui-test="Modal"]');
const container = document.getElementById('container');
const heading = document.getElementById('heading');

Expand Down
6 changes: 3 additions & 3 deletions packages/material-ui/src/Modal/TrapFocus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ function TrapFocus(props) {
open,
} = props;
const ignoreNextEnforceFocus = React.useRef();
const sentinelStart = React.useRef();
const sentinelEnd = React.useRef();
const sentinelStart = React.useRef(null);
const sentinelEnd = React.useRef(null);
const lastFocus = React.useRef();
const rootRef = React.useRef();
const rootRef = React.useRef(null);
// can be removed once we drop support for non ref forwarding class components
const handleOwnRef = React.useCallback(instance => {
// #StrictMode ready
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Popper/Popper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
transition,
...other
} = props;
const tooltipRef = React.useRef();
const tooltipRef = React.useRef(null);
const popperRef = React.useRef();
const [exited, setExited] = React.useState(!props.open);
const [placement, setPlacement] = React.useState();
Expand Down
88 changes: 28 additions & 60 deletions packages/material-ui/src/Portal/Portal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,82 +1,49 @@
import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import ownerDocument from '../utils/ownerDocument';
import { useForkRef } from '../utils/reactHelpers';
import { exactProp } from '@material-ui/utils';

function getContainer(container, defaultContainer) {
function getContainer(container) {
container = typeof container === 'function' ? container() : container;
return ReactDOM.findDOMNode(container) || defaultContainer;
return ReactDOM.findDOMNode(container);
}

function getOwnerDocument(element) {
return ownerDocument(ReactDOM.findDOMNode(element));
}
const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;

/**
* Portals provide a first-class way to render children into a DOM node
* that exists outside the DOM hierarchy of the parent component.
*/
class Portal extends React.Component {
componentDidMount() {
this.setMountNode(this.props.container);

// Only rerender if needed
if (!this.props.disablePortal) {
this.forceUpdate(this.props.onRendered);
const Portal = React.forwardRef(function Portal(props, ref) {
const { children, container, disablePortal, onRendered } = props;
const [mountNode, setMountNode] = React.useState(null);
const childRef = React.useRef(null);
Copy link
Collaborator

@joshwooding joshwooding Apr 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, do these explicitly need to be null?

Copy link
Member

@eps1lon eps1lon Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduces the number of overloads. React will set it to null when cleaning up so you end up with T | undefined | null if you don't initialize it. But I don't see a problem if you just compare weakly against nullish i.e. ref.current != null.

Copy link
Member

@oliviertassinari oliviertassinari Apr 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sebastian is right on spot. I have noticed the null / undefined difference in the tests that assert the ref values. It's inconsistent with the codebase. I'm reverting back to const childRef = React.useRef();. Thanks.

Copy link
Member

@oliviertassinari oliviertassinari Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, actually, I see a couple of different usage in the codebase:

  • React.useRef(undefined); x 1
  • React.useRef(null) x 10
  • React.useRef() x38

Copy link
Collaborator

@joshwooding joshwooding Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that’s why I asked :P As long as we stick to one I’m fine with either blank or null

Copy link
Member

@oliviertassinari oliviertassinari Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand Sebastian point, it's better in the demos to use null to reduce the number of types to write.

Copy link
Member

@oliviertassinari oliviertassinari Apr 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going with: null for the demos (improve TypeScript DX), nothing for the core (improve bundle size) and weak checks !=. It seems to be the already used standard, I only have to fix a few exceptions.

Copy link
Member

@eps1lon eps1lon Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For refs to components I would prefer useRef(null). This is equivalent to the old createRef style prevalent in classes:

const createdRef       = React.createRef(); //    createdRef.current === null
const usedRef          = React.useRef(null); //      usedRef.current === null
const usedUndefinedRef = React.useRef(); // usedUndefinedRef.current === undefined

Copy link
Member

@oliviertassinari oliviertassinari Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, good for me. The bundle size increase is negligeable. I will update the pull request to match createRef. It's simpler.

const handleRef = useForkRef(children.ref, childRef);

useEnhancedEffect(() => {
if (!disablePortal) {
setMountNode(getContainer(container) || document.body);
}
}

componentDidUpdate(prevProps) {
if (
prevProps.container !== this.props.container ||
prevProps.disablePortal !== this.props.disablePortal
) {
this.setMountNode(this.props.container);
}, [container]);

// Only rerender if needed
if (!this.props.disablePortal) {
this.forceUpdate(() => {
if (this.props.onRendered) {
// This might be triggered earlier than the componentDidUpdate of a parent element.
// We need to account for it.
clearTimeout(this.renderedTimer);
this.renderedTimer = setTimeout(this.props.onRendered);
}
});
}
React.useEffect(() => {
if (onRendered && mountNode) {
onRendered();
}
}

componentWillUnmount() {
this.mountNode = null;
clearTimeout(this.renderedTimer);
}
}, [mountNode, onRendered]);

setMountNode(container) {
if (this.props.disablePortal) {
this.mountNode = ReactDOM.findDOMNode(this).parentElement;
return;
}
React.useImperativeHandle(ref, () => mountNode || childRef.current, [mountNode]);

this.mountNode = getContainer(container, getOwnerDocument(this).body);
if (disablePortal) {
React.Children.only(children);
return React.cloneElement(children, {
ref: handleRef,
});
}

/**
* @public
*/
getMountNode = () => this.mountNode;

render() {
const { children, disablePortal } = this.props;

if (disablePortal) {
return children;
}

return this.mountNode ? ReactDOM.createPortal(children, this.mountNode) : null;
}
}
return mountNode ? ReactDOM.createPortal(children, mountNode) : mountNode;
});

Portal.propTypes = {
/**
Expand Down Expand Up @@ -106,7 +73,8 @@ Portal.defaultProps = {
};

if (process.env.NODE_ENV !== 'production') {
Portal.propTypes = exactProp(Portal.propTypes);
// eslint-disable-next-line
Portal['propTypes' + ''] = exactProp(Portal.propTypes);
}

export default Portal;
54 changes: 43 additions & 11 deletions packages/material-ui/src/Portal/Portal.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { assert } from 'chai';
import { spy } from 'sinon';
import PropTypes from 'prop-types';
import { createMount, createRender } from '@material-ui/core/test-utils';
import consoleErrorMock from 'test/utils/consoleErrorMock';
import Portal from './Portal';
import Select from '../Select';
import MenuItem from '../MenuItem';
Expand Down Expand Up @@ -86,6 +87,14 @@ describe('<Portal />', () => {
return;
}

beforeEach(() => {
consoleErrorMock.spy();
});

afterEach(() => {
consoleErrorMock.reset();
});

it('render nothing on the server', () => {
const markup1 = render(<div>Bar</div>);
assert.strictEqual(markup1.text(), 'Bar');
Expand All @@ -109,24 +118,39 @@ describe('<Portal />', () => {
});

it('should have access to the mountNode', () => {
const wrapper = mount(
<Portal>
const refSpy1 = spy();
mount(
<Portal ref={refSpy1}>
<h1>Foo</h1>
</Portal>,
);
const instance = wrapper.find('Portal').instance();
assert.strictEqual(instance.getMountNode(), instance.mountNode);
assert.deepEqual(refSpy1.args, [[null], [null], [document.body]]);
const refSpy2 = spy();
mount(
<Portal disablePortal ref={refSpy2}>
<h1 className="woofPortal">Foo</h1>
</Portal>,
);
assert.deepEqual(refSpy2.args, [[document.querySelector('.woofPortal')]]);
});

it('should render in a different node', () => {
const wrapper = mount(
<Portal>
<h1 className="woofPortal">Foo</h1>
</Portal>,
<div>
<h1 className="woofPortal1">Foo</h1>
<Portal>
<h1 className="woofPortal2">Foo</h1>
</Portal>
</div>,
);
assert.strictEqual(
wrapper.getDOMNode().contains(document.querySelector('.woofPortal1')),
true,
);
assert.strictEqual(
wrapper.getDOMNode().contains(document.querySelector('.woofPortal2')),
false,
);
const instance = wrapper.find('Portal').instance();
assert.notStrictEqual(instance.mountNode, null, 'should have a mountNode');
assert.strictEqual(document.querySelectorAll('.woofPortal').length, 1);
});

it('should unmount when parent unmounts', () => {
Expand Down Expand Up @@ -270,7 +294,15 @@ describe('<Portal />', () => {
wrapper.setProps({ container: null });

setTimeout(() => {
assert.deepEqual(callOrder, ['onRendered', 'cDU', 'cDU', 'cDU', 'onRendered']);
assert.deepEqual(callOrder, [
'onRendered',
'cDU',
'onRendered',
'cDU',
'onRendered',
'cDU',
'onRendered',
]);
done();
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/RadioGroup/RadioGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import RadioGroupContext from './RadioGroupContext';

const RadioGroup = React.forwardRef(function RadioGroup(props, ref) {
const { actions, children, name, value: valueProp, onChange, ...other } = props;
const rootRef = React.useRef();
const rootRef = React.useRef(null);
const { current: isControlled } = React.useRef(props.value != null);
const [valueState, setValue] = React.useState(() => {
if (!isControlled) {
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Select/SelectInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
variant,
...other
} = props;
const displayRef = React.useRef();
const displayRef = React.useRef(null);
const ignoreNextBlur = React.useRef(false);
const { current: isOpenControlled } = React.useRef(props.open != null);
const [menuMinWidthState, setMenuMinWidthState] = React.useState();
Expand Down
Loading