Material-ui: [Popover] Reposition when rerender

[KevinAsher]

• [x] 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]

@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:

• https://caniuse.com/#feat=resizeobserver (not supported everywhere yet)
• https://github.com/que-etc/resize-observer-polyfill (bundle size overhead implication)

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

[smsteel]

How to use this?

[joshwooding]

@smsteel @bjm904 https://codesandbox.io/s/32wxjrq3op

[Dzheky]

Hello is there any progress on this issue?

[oliviertassinari]

@Dzheky Did you manage to use the suggested workaround? It's basically the same problem as with #9337.

[Dzheky]

@oliviertassinari I've read through the issue you linked and I didn't found the suggested workaround for this particular issue

[cadbox1]

I'll clarify how I implemented the suggested solution. You can work it out by looking at the code changes in the pull request, particularly the test.

I actually didn't end up using this solution because my Popover expanded based on user input which meant it jumped around which I didn't like so I just set a height on the contents with an overflow but I think this solution would work quite well for a dynamic load or something similar.

didn't run this code but I think it demonstrates the point.

class MyPopover extends Component {
    constructor() {
        this.state = {
            itemCount: 0
        }
    }
    componentDidMount() {
        setTimeout(() => this.setState({ itemCount: 100 }), 1000)
    }
    componentDidUpdate() {
        if (this.popoverActions) {
            this.popoverActions.updatePosition();
        }
    }
    render() {
        const items = [];
        for (let i=0; i<this.state.itemCount; i++) {
            items.push(<p>item</p>)
        }
        return <Popover
            action={actions => {
                this.popoverActions = actions
            }}
        >
            {items}
        </Popover>
    }
}

[taqimustafa]

Any good solution of how we can use this with hooks ?

[jessecoleman]

Here was my solution with hooks

const MyPopover = () => {

    const [ itemCount, setItemCount ] = useState(0);
    const popoverActions = useRef(null);

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

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

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

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

Using the useRef hook instead of useState to store the actions function is necessary to avoid an infinite rendering loop.

[oliviertassinari]

We should be able to apply a similar fix than https://github.com/mui-org/material-ui/issues/18310#issuecomment-552426596. 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? :)

[SandraMarcelaHerreraArriaga]

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

[mbrookes]

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