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.

Sign up for GitHub

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

Jump to bottom

[Tooltip] - PopperProps broken starting from 4.9.5 #20095

Closed
2 tasks done
ValentinH opened this issue Mar 13, 2020 · 2 comments · Fixed by #20100
Closed
2 tasks done

[Tooltip] - PopperProps broken starting from 4.9.5 #20095

ValentinH opened this issue Mar 13, 2020 · 2 comments · Fixed by #20100
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module!

Comments

@ValentinH
Copy link
Contributor

Because of #19851, we are not able to create our own custom arrow for a Tooltip.
image

  • 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 😯

Passing an arrow modifier to PopperProps, throws Failed to execute 'contains' on 'Node': parameter 1 is not of type 'Node'.

Expected Behavior 🤔

The arrow is displayed properly.

Steps to Reproduce 🕹

Working version (4.9.4): https://codesandbox.io/s/material-demo-ltrt5
Broken version (4.9.5): https://codesandbox.io/s/material-demo-ddi25

Root cause

This is because deepmerge() replaces DOM nodes by {} objects.

Here a screenshot of the following console.log:

image
image

Your Environment 🌎

Tech Version
Material-UI v4.9.5
React v16.13.0
Browser Brave Version 1.3.118 Chromium: 80.0.3987.116 (Official Build) (64-bit)
@eps1lon
Copy link
Member

eps1lon commented Mar 13, 2020

It looks like it's actually attempting to merge the Elements. We probably just want to merge plain objects.

@eps1lon eps1lon added bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. component: tooltip This is the name of the generic UI component, not the React module! and removed component: Popper The React component. See <Popup> for the latest version. labels Mar 13, 2020
@ValentinH
Copy link
Contributor Author

Should I create a PR to prevent deepMerge from merging Elements?

ValentinH pushed a commit to ValentinH/material-ui that referenced this issue Mar 13, 2020
deepmerge: do not merge DOM elements
055a349 
Fixes mui#20095
@ValentinH ValentinH mentioned this issue Mar 13, 2020
Merged
1 task
oliviertassinari pushed a commit to ValentinH/material-ui that referenced this issue Mar 15, 2020
@oliviertassinari
deepmerge: do not merge DOM elements
360c457 
Fixes mui#20095
This was referenced Nov 26, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[core] Fix deepmerge of DOM elements ValentinH/material-ui
2 participants
@ValentinH @eps1lon