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

[Popover] Add warning when non-ref-holding component is used in Paper #15181

Merged
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
7 changes: 5 additions & 2 deletions packages/material-ui/src/Popover/Popover.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import warning from 'warning';
import debounce from 'debounce'; // < 1kb payload overhead when lodash/debounce is > 3kb.
import EventListener from 'react-event-listener';
import clsx from 'clsx';
import { chainPropTypes } from '@material-ui/utils';
import { chainPropTypes, elementTypeAcceptingRef } from '@material-ui/utils';
import ownerDocument from '../utils/ownerDocument';
import ownerWindow from '../utils/ownerWindow';
import { createChainedFunction } from '../utils/helpers';
Expand Down Expand Up @@ -348,6 +348,7 @@ class Popover extends React.Component {
data-mui-test="Popover"
elevation={elevation}
ref={ref => {
// #StrictMode ready
this.paperRef = ReactDOM.findDOMNode(ref);
}}
{...PaperProps}
Expand Down Expand Up @@ -521,7 +522,9 @@ Popover.propTypes = {
/**
* Properties applied to the [`Paper`](/api/paper/) element.
*/
PaperProps: PropTypes.object,
PaperProps: PropTypes.shape({
component: elementTypeAcceptingRef,
}),
/**
* This is the point on the popover which
* will attach to the anchor's origin.
Expand Down
9 changes: 9 additions & 0 deletions packages/material-ui/src/Popover/Popover.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,15 @@ describe('<Popover />', () => {
assert.include(consoleErrorMock.args()[0][0], 'It should be a HTMLElement instance');
});

it('warns if a component for the Paper is used that cant hold a ref', () => {
mount(<Popover {...defaultProps} PaperProps={{ component: () => <div />, elevation: 4 }} />);
assert.strictEqual(consoleErrorMock.callCount(), 1);
assert.include(
consoleErrorMock.args()[0][0],
'Warning: Failed prop type: Invalid prop `PaperProps.component` supplied to `Popover`. Expected an element type that can hold a ref.',
);
});

// it('should warn if anchorEl is not visible', () => {
// mount(<Popover open anchorEl={document.createElement('div')} />);
// assert.strictEqual(consoleErrorMock.callCount(), 1);
Expand Down
17 changes: 13 additions & 4 deletions packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import React from 'react';
import PropTypes from 'prop-types';
import ReactDOM from 'react-dom';
import { elementTypeAcceptingRef } from '@material-ui/utils';
import Drawer, { getAnchor, isHorizontal } from '../Drawer/Drawer';
import { duration } from '../styles/transitions';
import withTheme from '../styles/withTheme';
Expand Down Expand Up @@ -327,11 +328,13 @@ class SwipeableDrawer extends React.Component {
};

handleBackdropRef = ref => {
this.backdropRef = ref ? ReactDOM.findDOMNode(ref) : null;
// #StrictMode ready
this.backdropRef = ReactDOM.findDOMNode(ref);
};

handlePaperRef = ref => {
this.paperRef = ref ? ReactDOM.findDOMNode(ref) : null;
// #StrictMode ready
this.paperRef = ReactDOM.findDOMNode(ref);
};

listenTouchStart() {
Expand Down Expand Up @@ -450,7 +453,11 @@ SwipeableDrawer.propTypes = {
/**
* @ignore
*/
ModalProps: PropTypes.object,
ModalProps: PropTypes.shape({
BackdropProps: PropTypes.shape({
component: elementTypeAcceptingRef,
}),
}),
/**
* Callback fired when the component requests to be closed.
*
Expand All @@ -470,7 +477,9 @@ SwipeableDrawer.propTypes = {
/**
* @ignore
*/
PaperProps: PropTypes.object,
PaperProps: PropTypes.shape({
component: elementTypeAcceptingRef,
}),
/**
* Properties applied to the swipe area element.
*/
Expand Down
47 changes: 47 additions & 0 deletions packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import React from 'react';
import { assert } from 'chai';
import { spy, stub } from 'sinon';
import { createMount, describeConformance, unwrap } from '@material-ui/core/test-utils';
import PropTypes from 'prop-types';
import consoleErrorMock from 'test/utils/consoleErrorMock';
import Drawer from '../Drawer';
import SwipeableDrawer, { reset } from './SwipeableDrawer';
import SwipeArea from './SwipeArea';
Expand Down Expand Up @@ -526,4 +528,49 @@ describe('<SwipeableDrawer />', () => {
fireSwipeAreaMouseEvent(wrapper, 'touchstart', { touches: [{ pageX: 0, clientY: 0 }] });
});
});

describe('warnings', () => {
beforeEach(() => {
consoleErrorMock.spy();
PropTypes.resetWarningCache();
});

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

it('warns if a component for the Paper is used that cant hold a ref', () => {
mount(
<SwipeableDrawer
onOpen={() => {}}
onClose={() => {}}
open={false}
PaperProps={{ component: () => <div />, elevation: 4 }}
/>,
);

assert.strictEqual(consoleErrorMock.callCount(), 1);
assert.include(
consoleErrorMock.args()[0][0],
'Warning: Failed prop type: Invalid prop `PaperProps.component` supplied to `SwipeableDrawer`. Expected an element type that can hold a ref.',
);
});

it('warns if a component for the Backdrop is used that cant hold a ref', () => {
mount(
<SwipeableDrawer
onOpen={() => {}}
onClose={() => {}}
open={false}
ModalProps={{ BackdropProps: { component: () => <div />, 'data-backdrop': true } }}
/>,
);

assert.strictEqual(consoleErrorMock.callCount(), 1);
assert.include(
consoleErrorMock.args()[0][0],
'Warning: Failed prop type: Invalid prop `ModalProps.BackdropProps.component` supplied to `SwipeableDrawer`. Expected an element type that can hold a ref.',
);
});
});
});
2 changes: 1 addition & 1 deletion pages/api/popover.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import Popover from '@material-ui/core/Popover';
| <span class="prop-name">onExited</span> | <span class="prop-type">func</span> | | Callback fired when the component has exited. |
| <span class="prop-name">onExiting</span> | <span class="prop-type">func</span> | | Callback fired when the component is exiting. |
| <span class="prop-name required">open&nbsp;*</span> | <span class="prop-type">bool</span> | | If `true`, the popover is visible. |
| <span class="prop-name">PaperProps</span> | <span class="prop-type">object</span> | | Properties applied to the [`Paper`](/api/paper/) element. |
| <span class="prop-name">PaperProps</span> | <span class="prop-type">{ component?: element type }</span> | | Properties applied to the [`Paper`](/api/paper/) element. |
| <span class="prop-name">transformOrigin</span> | <span class="prop-type">{ horizontal: union:&nbsp;number&nbsp;&#124;<br>&nbsp;enum:&nbsp;'left'&nbsp;&#124;<br>&nbsp;'center'&nbsp;&#124;<br>&nbsp;'right'<br><br>, vertical: union:&nbsp;number&nbsp;&#124;<br>&nbsp;enum:&nbsp;'top'&nbsp;&#124;<br>&nbsp;'center'&nbsp;&#124;<br>&nbsp;'bottom'<br><br> }</span> | <span class="prop-default">{ vertical: 'top', horizontal: 'left',}</span> | This is the point on the popover which will attach to the anchor's origin.<br>Options: vertical: [top, center, bottom, x(px)]; horizontal: [left, center, right, x(px)]. |
| <span class="prop-name">TransitionComponent</span> | <span class="prop-type">elementType</span> | <span class="prop-default">Grow</span> | The component used for the transition. |
| <span class="prop-name">transitionDuration</span> | <span class="prop-type">union:&nbsp;number&nbsp;&#124;<br>&nbsp;{ enter?: number, exit?: number }&nbsp;&#124;<br>&nbsp;enum:&nbsp;'auto'<br><br></span> | <span class="prop-default">'auto'</span> | Set to 'auto' to automatically calculate transition time based on height. |
Expand Down