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] Reposition when rerender #10595

Closed
1 task done
KevinAsher opened this issue Mar 9, 2018 · 12 comments · Fixed by #19046
Closed
1 task done

[Popover] Reposition when rerender #10595

KevinAsher opened this issue Mar 9, 2018 · 12 comments · Fixed by #19046
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

@KevinAsher
Copy link
Contributor

KevinAsher commented Mar 9, 2018

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

Expected Behavior

If the <Select> component is opened, and we are loading its items dynamically, it's expected that when the items are finally loaded the <Select>'s opened overlay adjusts itself to the current available space.

Current Behavior

The <Select>'s overlay overflows.

Steps to Reproduce (for bugs)

In the sandbox, click on the <Select> and wait for it to load.

https://codesandbox.io/s/vqvm1704q3

Your Environment

Tech Version
Material-UI 1.0.0-beta.35
React 16.2.0
browser Google Chrome 64.0.3282.186
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! labels Mar 9, 2018
@oliviertassinari oliviertassinari added this to the post v1.0.0 milestone Mar 9, 2018
@oliviertassinari oliviertassinari changed the title Dynamic Contents with Select - Overlay Overflow [Popover] Dynamic contents Apr 4, 2018
@oliviertassinari oliviertassinari added component: Popover The React component. and removed component: select This is the name of the generic UI component, not the React module! labels Apr 4, 2018
@oliviertassinari oliviertassinari added discussion and removed bug 🐛 Something doesn't work labels Jul 6, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 6, 2018

@KevinAsher We have added an action prop to the Popover component since to solve the problem. You can trigger the .updatePosition() method when you need to update the position of the Popover (it was added in #9588)

For instance:

function MyPopover() {
  const [itemCount, setItemCount] = React.useState(0);
  const popoverActions = React.useRef();

  React.useEffect(() => {
    setTimeout(() => setItemCount(100), 1000);
  }, []);

  React.useEffect(() => {
    if (popoverActions.current) {
      popoverActions.current.updatePosition();
    }
  }, [itemCount]);

  const items = [];
  for (let i = 0; i < itemCount; i++) {
    items.push(<p key={i}>item</p>);
  }

  return <Popover action={popoverActions}>{items}</Popover>;
}

However, it might require too much boilerplate, a workaround is to trigger a window resize event (don't abuse it!):

window.dispatchEvent(new CustomEvent('resize'))

In the future, we should consider using the ResizeObserver API for out of the box support:

Alternatively, we can consider updating the position at each render. It could be a low hanging fruit :).

@smsteel

This comment has been minimized.

@joshwooding

This comment has been minimized.

@Dzheky

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@Dzheky

This comment has been minimized.

@cadbox1

This comment has been minimized.

@taqimustafa

This comment has been minimized.

@jessecoleman

This comment has been minimized.

@oliviertassinari oliviertassinari added new feature New feature or request and removed discussion labels Sep 23, 2019
@oliviertassinari oliviertassinari changed the title [Popover] Dynamic contents [Popover] Update position upon content resize Sep 23, 2019
@oliviertassinari oliviertassinari added the waiting for 👍 Waiting for upvotes label Sep 23, 2019
@oliviertassinari oliviertassinari removed the waiting for 👍 Waiting for upvotes label Nov 30, 2019
@oliviertassinari oliviertassinari changed the title [Popover] Update position upon content resize [Popover] Reposition when rerender Nov 30, 2019
@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Nov 30, 2019
@oliviertassinari
Copy link
Member

We should be able to apply a similar fix than #18310 (comment). I would propose something close to:

diff --git a/packages/material-ui/src/Popover/Popover.js b/packages/material-ui/src/Popover/Popover.js
index 9f23c25e2..722ee83b6 100644
--- a/packages/material-ui/src/Popover/Popover.js
+++ b/packages/material-ui/src/Popover/Popover.js
@@ -335,32 +335,40 @@ const Popover = React.forwardRef(function Popover(props, ref) {
     paperRef.current = ReactDOM.findDOMNode(instance);
   }, []);

-  const updatePosition = React.useMemo(() => {
-    if (!open) {
-      return undefined;
-    }
-
-    return debounce(() => {
+  React.useEffect(() => {
+    if (open && paperRef.current) {
       setPositioningStyles(paperRef.current);
-    });
-  }, [open, setPositioningStyles]);
+    }
+  });

-  React.useImperativeHandle(action, () => (open ? { updatePosition } : null), [
-    open,
-    updatePosition,
-  ]);
+  React.useImperativeHandle(
+    action,
+    () =>
+      open
+        ? {
+            updatePosition: () => {
+              setPositioningStyles(paperRef.current);
+            },
+          }
+        : null,
+    [open, setPositioningStyles],
+  );

   React.useEffect(() => {
-    if (!updatePosition) {
+    if (!open) {
       return undefined;
     }

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

   let transitionDuration = transitionDurationProp;

Does somebody want to work on a pull request? :)

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed new feature New feature or request labels Nov 30, 2019
@SandraMarcelaHerreraArriaga
Copy link
Contributor

Hello @oliviertassinari can I start to work on this issue? :)

@mbrookes
Copy link
Member

@SandraMarcelaHerreraArriaga No permission needed. Thanks for working on it!

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.

10 participants