So, I feel like I did a bad job of making my case for why I made PopoverContainer. My main goal was to abstract over what I think is a bad API from MUI – I think it encourages a lot of patterns that break React's rules, and in the future, run the risk of breaking our entire app.

Here's the link to the previous PR, where I mentioned some of the problems, but here's also a quote from the React docs page for useRef:

Do not write or read ref.current during rendering, except for initialization. This makes your component’s behavior unpredictable.

My worry is that the React team is pushing for an approach of "we reserve the right to change how things work under the hood at any time, and the only way to not get bitten is by following our rules 100%". We've already seen this with React.StrictMode breaking other companies' apps, since many weren't built to be double-called in dev mode. We're going to be getting an update in the next few days on the new React Forget compiler – there's a chance that might also "punish" more patterns that we're using right now. The React team also hasn't been afraid to break MUI itself – it still can't be used in React Server Components.

Plus, I know you've said that you've used Radix before. I'm sure you can appreciate how much simpler its Popover component is:

import * as Popover from '@radix-ui/react-popover';

export default () => (
  <Popover.Root>
    <Popover.Trigger />
    <Popover.Anchor />
    <Popover.Portal>
      <Popover.Content>
        <Popover.Close />
        <Popover.Arrow />
      </Popover.Content>
    </Popover.Portal>
  </Popover.Root>
);

Unless you have a really powerful use case, there's no need for hooks or other state to make the component function – everything just works via composition.

I think I had two goals with the component:

1. Massage MUI's API to work strictly through composition. I'm of the mind that you can't get rid of complexity – you can only shift it around. So I'd rather something be complicated internally, than make things complicated for anything consuming it
2. Write things in a way that insulates our codebase from these potential breaking changes in the future

To be clear, your styling changes are definitely better, and the horizontal spacing is working more consistently. And the disablePortal property helps remove the need for these dedicated links. I'm just worried that MUI's Popover could become an albatross for us down the line

But you also know more way more about the codebase than I do, so I'm going to trust whatever you think is best.

I'd be okay with accepting this PR right now, and then possibly revisiting PopoverContainer to redesign it, and get the edge cases/design issues ironed out

This seems like a much nicer solution than my forced closing approach, but I did notice this from the MUI docs:

The children is Portal to the body of the document to avoid rendering problems. You can disable this behavior with disablePortal.

I couldn't find anything about what these rendering problems are, though. Do you happen to know anything about this?

I never had such problems but I would guess it can be related to style since the component would be rendered inside of the component scope and not outside of the #root element

What are your thoughts on replacing this with padding (probably from a parent element), just to avoid any risks of margin collapse in the future?

I see, I think it makes sense.

I feel like this component probably doesn't make sense now, since we have the disablePortal prop

I only needed this as a separate component, so I could have a new component boundary to call hooks from, and get closePopover via context