Skip to content

Commit

Permalink
[Popper] Fix anchorEl prop types (#16004)
Browse files Browse the repository at this point in the history
* Fix Popper's anchorEl prop type

* make the visibility check work with referenceObject

* better warning message

* revert check visibility on referenceObject

* refine the wording
  • Loading branch information
dan8f authored and oliviertassinari committed Jun 8, 2019
1 parent 7199660 commit 41f1722
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 12 deletions.
6 changes: 2 additions & 4 deletions docs/src/pages/components/popper/SimplePopper.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const useStyles = makeStyles(theme => ({
},
}));

function SimplePopper() {
export default function SimplePopper() {
const classes = useStyles();
const [anchorEl, setAnchorEl] = React.useState(null);

Expand All @@ -21,7 +21,7 @@ function SimplePopper() {
}

const open = Boolean(anchorEl);
const id = open ? 'simple-popper' : null;
const id = open ? 'simple-popper' : undefined;

return (
<div>
Expand All @@ -40,5 +40,3 @@ function SimplePopper() {
</div>
);
}

export default SimplePopper;
44 changes: 44 additions & 0 deletions docs/src/pages/components/popper/SimplePopper.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import React from 'react';
import { makeStyles, Theme, createStyles } from '@material-ui/core/styles';
import Popper from '@material-ui/core/Popper';
import Typography from '@material-ui/core/Typography';
import Button from '@material-ui/core/Button';
import Fade from '@material-ui/core/Fade';
import Paper from '@material-ui/core/Paper';

const useStyles = makeStyles((theme: Theme) =>
createStyles({
typography: {
padding: theme.spacing(2),
},
}),
);

export default function SimplePopper() {
const classes = useStyles();
const [anchorEl, setAnchorEl] = React.useState<null | HTMLElement>(null);

function handleClick(event: React.MouseEvent<HTMLElement>) {
setAnchorEl(anchorEl ? null : event.currentTarget);
}

const open = Boolean(anchorEl);
const id = open ? 'simple-popper' : undefined;

return (
<div>
<Button aria-describedby={id} variant="contained" onClick={handleClick}>
Toggle Popper
</Button>
<Popper id={id} open={open} anchorEl={anchorEl} transition>
{({ TransitionProps }) => (
<Fade {...TransitionProps} timeout={350}>
<Paper>
<Typography className={classes.typography}>The content of the Popper.</Typography>
</Paper>
</Fade>
)}
</Popper>
</div>
);
}
2 changes: 1 addition & 1 deletion docs/src/pages/components/popper/popper.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Some important features of the `Popper` component:

- 🕷 Popper relies on the 3rd party library ([Popper.js](https://github.com/FezVrasta/popper.js)) for perfect positioning.
- 💄 It's an alternative API to react-popper. It aims for simplicity.
- 📦 [10 kB gzipped](/size-snapshot).
- 📦 [10 kB gzipped](/size-snapshot) (7 kB from Popper.js).
- The children is [`Portal`](/components/portal/) to the body of the document to avoid rendering problems.
You can disable this behavior with `disablePortal`.
- The scroll isn't blocked like with the [`Popover`](/components/popover/) component.
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Popper/Popper.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export type PopperPlacementType =

export interface PopperProps extends React.HTMLAttributes<HTMLDivElement> {
transition?: boolean;
anchorEl?: null | Element | ReferenceObject | (() => Element);
anchorEl?: null | ReferenceObject | (() => ReferenceObject);
children:
| React.ReactNode
| ((props: {
Expand Down
18 changes: 14 additions & 4 deletions packages/material-ui/src/Popper/Popper.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,13 @@ const Popper = React.forwardRef(function Popper(props, ref) {

Popper.propTypes = {
/**
* This is the DOM element, or a function that returns the DOM element,
* This is the reference element, or a function that returns the reference element,
* that may be used to set the position of the popover.
* The return value will passed as the reference object of the Popper
* instance.
*
* The reference element should be an HTML Element instance or a referenceObject:
* https://popper.js.org/popper-documentation.html#referenceObject.
*/
anchorEl: chainPropTypes(PropTypes.oneOfType([PropTypes.object, PropTypes.func]), props => {
if (props.open) {
Expand All @@ -187,15 +190,22 @@ Popper.propTypes = {
return new Error(
[
'Material-UI: the `anchorEl` prop provided to the component is invalid.',
'The node element should be visible.',
'The reference element should be part of the document layout.',
"Make sure the element is present in the document or that it's not display none.",
].join('\n'),
);
}
} else {
} else if (
!resolvedAnchorEl ||
typeof resolvedAnchorEl.clientWidth !== 'number' ||
typeof resolvedAnchorEl.clientHeight !== 'number' ||
typeof resolvedAnchorEl.getBoundingClientRect !== 'function'
) {
return new Error(
[
'Material-UI: the `anchorEl` prop provided to the component is invalid.',
`It should be an Element instance but it's \`${resolvedAnchorEl}\` instead.`,
'It should be an HTML Element instance or a referenceObject:',
'https://popper.js.org/popper-documentation.html#referenceObject.',
].join('\n'),
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Popper/Popper.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ describe('<Popper />', () => {
it('should warn if anchorEl is not valid', () => {
mount(<Popper {...defaultProps} open anchorEl={null} />);
assert.strictEqual(consoleErrorMock.callCount(), 1);
assert.include(consoleErrorMock.args()[0][0], 'It should be an Element instance');
assert.include(consoleErrorMock.args()[0][0], 'It should be an HTML Element instance');
});

// it('should warn if anchorEl is not visible', () => {
Expand Down
2 changes: 1 addition & 1 deletion pages/api/popper.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Poppers rely on the 3rd party library [Popper.js](https://github.com/FezVrasta/p

| Name | Type | Default | Description |
|:-----|:-----|:--------|:------------|
| <span class="prop-name">anchorEl</span> | <span class="prop-type">union:&nbsp;object&nbsp;&#124;<br>&nbsp;func<br></span> | | This is the DOM element, or a function that returns the DOM element, that may be used to set the position of the popover. The return value will passed as the reference object of the Popper instance. |
| <span class="prop-name">anchorEl</span> | <span class="prop-type">union:&nbsp;object&nbsp;&#124;<br>&nbsp;func<br></span> | | This is the reference element, or a function that returns the reference element, that may be used to set the position of the popover. The return value will passed as the reference object of the Popper instance.<br>The reference element should be an HTML Element instance or a referenceObject: https://popper.js.org/popper-documentation.html#referenceObject. |
| <span class="prop-name required">children&nbsp;*</span> | <span class="prop-type">union:&nbsp;node&nbsp;&#124;<br>&nbsp;func<br></span> | | Popper render function or node. |
| <span class="prop-name">container</span> | <span class="prop-type">union:&nbsp;object&nbsp;&#124;<br>&nbsp;func<br></span> | | A node, component instance, or function that returns either. The `container` will passed to the Modal component. By default, it uses the body of the anchorEl's top-level document object, so it's simply `document.body` most of the time. |
| <span class="prop-name">disablePortal</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | Disable the portal behavior. The children stay within it's parent DOM hierarchy. |
Expand Down

0 comments on commit 41f1722

Please sign in to comment.