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

Fix mui Popover integration #2403

Merged
merged 6 commits into from
Mar 10, 2020
Merged

Fix mui Popover integration #2403

merged 6 commits into from
Mar 10, 2020

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Mar 10, 2020

The problem

In mui there's a concept of forking a ref, this means that we combine the passed ref from the top with the one used internally inside a component to manage presentation.

An example: https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Grow/Grow.js#L32

This becomes problematic in the following scenario

const Wrapper = forwardRef((props, ref) => <div ref={ref}>I am wrapped</div>);
const App = () => {
  const ref = useRef()
  return (
    <Grow>
       <Wrapper ref={ref} />
    </Grow>
  )
}

At this point we have evaluated Wrapper and moved the vnode.ref to vnode.props.ref due to options.vnode being called (createVNode). This makes the forking of the Grow ref fail since we talk to children.ref which in this case is Wrapper.ref. That's why we move the forwarding of refs to the point where we are going to diff the forwarded component.

Fixes: #2249

@github-actions
Copy link

github-actions bot commented Mar 10, 2020

Size Change: +6 B (0%)

Total Size: 38.5 kB

Filename Size Change
compat/dist/compat.js 3.12 kB +3 B (0%)
compat/dist/compat.module.js 3.14 kB +1 B
compat/dist/compat.umd.js 3.17 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
debug/dist/debug.js 2.98 kB 0 B
debug/dist/debug.module.js 2.96 kB 0 B
debug/dist/debug.umd.js 3.04 kB 0 B
devtools/dist/devtools.js 175 B 0 B
devtools/dist/devtools.module.js 185 B 0 B
devtools/dist/devtools.umd.js 252 B 0 B
dist/preact.js 3.72 kB 0 B
dist/preact.min.js 3.72 kB 0 B
dist/preact.module.js 3.73 kB 0 B
dist/preact.umd.js 3.78 kB 0 B
hooks/dist/hooks.js 1.05 kB 0 B
hooks/dist/hooks.module.js 1.08 kB 0 B
hooks/dist/hooks.umd.js 1.13 kB 0 B
test-utils/dist/testUtils.js 390 B 0 B
test-utils/dist/testUtils.module.js 392 B 0 B
test-utils/dist/testUtils.umd.js 469 B 0 B

compressed-size-action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Material-UI 4: popover loose positioning
2 participants