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] updatePosition action doesn't work #16901

Closed
2 tasks done
DominikSerafin opened this issue Aug 5, 2019 · 3 comments · Fixed by #17097
Closed
2 tasks done

[Popover] updatePosition action doesn't work #16901

DominikSerafin opened this issue Aug 5, 2019 · 3 comments · Fixed by #17097
Labels
bug 🐛 Something doesn't work component: Popover The React component. good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@DominikSerafin
Copy link
Contributor

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Popover position updates nicely on window resize, however when I want to update it manually via updatePosition() - it doesn't work.

Expected Behavior 🤔

The position should update when updatePosition() is called.

Steps to Reproduce 🕹

https://codesandbox.io/s/material-demo-rd6dy

  1. Click "Open Popover"
  2. Click "Show More"
  3. The Popover content/paper should be at this stage cut at the bottom
  4. Click "Update Position"
  5. Nothing happens

Context 🔦

Your Environment 🌎

Tech Version
Material-UI v4.2.* & v4.3.*
React
Browser
TypeScript
etc.
@oliviertassinari oliviertassinari added component: Popover The React component. bug 🐛 Something doesn't work labels Aug 5, 2019
@DominikSerafin
Copy link
Contributor Author

I'm not entirely sure, but I think this broke somewhere around rewrite into hooks

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 5, 2019

@DominikSerafin I confirm, we have a couple of issues with the logic. What about the following strategy?

diff --git a/packages/material-ui/src/Popover/Popover.js b/packages/material-ui/src/Popover/Popover.js
index fbf5cf29c..d4de66ea6 100644
--- a/packages/material-ui/src/Popover/Popover.js
+++ b/packages/material-ui/src/Popover/Popover.js
@@ -115,15 +115,6 @@ const Popover = React.forwardRef(function Popover(props, ref) {
     ...other
   } = props;
   const paperRef = React.useRef();
-  const handleResizeRef = React.useRef(() => {});
-
-  React.useImperativeHandle(
-    action,
-    () => ({
-      updatePosition: handleResizeRef.current,
-    }),
-    [],
-  );

   // Returns the top/left offset of the position
   // to attach to on the anchor element (or body if none is provided)
@@ -297,6 +288,29 @@ const Popover = React.forwardRef(function Popover(props, ref) {
     [getPositioningStyle],
   );

+  const updatePosition = React.useMemo(() => {
+    if (!open) {
+      return undefined;
+    }
+
+    return debounce(() => {
+      setPositioningStyles(paperRef.current);
+    });
+  }, [open, setPositioningStyles]);
+
+  React.useImperativeHandle(
+    action,
+    () =>
+      open
+        ? {
+            updatePosition,
+          }
+        : null,
+    [open, updatePosition],
+  );
+
   const handleEntering = element => {
     if (onEntering) {
       onEntering(element);
@@ -311,21 +325,18 @@ const Popover = React.forwardRef(function Popover(props, ref) {
   }, []);

   React.useEffect(() => {
-    handleResizeRef.current = debounce(() => {
-      // Because we debounce the event, the open prop might no longer be true
-      // when the callback resolves.
-      if (!open) {
-        return;
-      }
+    if (!updatePosition) {
+      return undefined;
+    }

-      setPositioningStyles(paperRef.current);
-    });
-    window.addEventListener('resize', handleResizeRef.current);
+    window.addEventListener('resize', updatePosition);
     return () => {
-      handleResizeRef.current.clear();
-      window.removeEventListener('resize', handleResizeRef.current);
+      window.removeEventListener('resize', updatePosition);
+      updatePosition.clear();
     };
-  }, [open, setPositioningStyles]);
+  }, [updatePosition]);

   let transitionDuration = transitionDurationProp;

@@ -384,7 +395,7 @@ Popover.propTypes = {
    * @param {object} actions This object contains all possible actions
    * that can be triggered programmatically.
    */
-  action: PropTypes.func,
+  action: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
   /**
    * This is the DOM element, or a function that returns the DOM element,
    * that may be used to set the position of the popover.

@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. and removed help appreciated labels Aug 15, 2019
@netochaves
Copy link
Contributor

@oliviertassinari i test u strategy and works fine, can i open a pr?

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: Popover The React component. 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