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

Popper faked reference object props validation fails #15922

Closed
2 tasks done
dan8f opened this issue May 28, 2019 · 2 comments · Fixed by #16004
Closed
2 tasks done

Popper faked reference object props validation fails #15922

dan8f opened this issue May 28, 2019 · 2 comments · Fixed by #16004
Labels
bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@dan8f
Copy link
Contributor

dan8f commented May 28, 2019

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

anchorEl prop to validate when a faked reference object is passed.

Current Behavior 😯

Validation fails.

"Warning: Failed prop type: Material-UI: the anchorEl prop provided to the component is invalid.
It should be an Element instance but it's [object Object] instead."

Steps to Reproduce 🕹

Check the documentation example: https://material-ui.com/components/popper/#faked-reference-object

The codesandbox in the documentation reproduces the failure https://codesandbox.io/s/hp8d5

Context 🔦

Your Environment 🌎

Tech Version
Material-UI v4.0.1
React 16.8.1
Browser
TypeScript
etc.
@eps1lon eps1lon added bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. labels May 29, 2019
@eps1lon
Copy link
Member

eps1lon commented May 29, 2019

Sounds like PropTypes.shap would be the better candidate here. Our typescript types need adjustment as well though and I'm not sure where we draw the boundary. If we only require the properties we currently use we might block important bug fixes in the future (layout computation is always a bit tricky across browsers).

Would like to see a proposal for a shape that we require in anchorEl and then we can discuss what we should add just to be safe.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 1, 2019

@dan8f Thanks for raising the problem. Would you be interested in opening a pull request?
@eps1lon I would propose the following diff. Does it look good to you?:

diff --git a/packages/material-ui/src/Popper/Popper.d.ts b/packages/material-ui/src/Popper/Popper.d.ts
index 2e005a805..511a9d492 100644
--- a/packages/material-ui/src/Popper/Popper.d.ts
+++ b/packages/material-ui/src/Popper/Popper.d.ts
@@ -19,7 +19,7 @@ export type PopperPlacementType =

 export interface PopperProps extends React.HTMLAttributes<HTMLDivElement> {
   transition?: boolean;
-  anchorEl?: null | Element | ReferenceObject | (() => Element);
+  anchorEl?: null | Element | ReferenceObject | (() => Element | ReferenceObject);
   children:
     | React.ReactNode
     | ((props: {
diff --git a/packages/material-ui/src/Popper/Popper.js b/packages/material-ui/src/Popper/Popper.js
index 7d48d99ae..97a4a9f13 100644
--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -191,11 +191,16 @@ Popper.propTypes = {
             ].join('\n'),
           );
         }
-      } else {
+      } else if (
+        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 Element instance or a referenceObject:',
+            'https://popper.js.org/popper-documentation.html#referenceObject.',
           ].join('\n'),
         );
       }

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Jun 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants