[Popper] Fix deep merge of PopperProps #19851
Conversation
valgrindMaster
changed the title
Details of bundle changes.Comparing: aa09632...0c7d2bb
|
eps1lon
changed the title
eps1lon
added
bug 🐛
| @@ -397,6 +397,68 @@ describe('<Tooltip />', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('prop: PopperProps', () => { | |||
| it('should pass PopperProps to Popper Component', () => { | |||
| const wrapper = mount(<Tooltip {...defaultProps} PopperProps={{ item: 'value' }} />); | |||
There was a problem hiding this comment.
What do you think of using the render API instead (createClientRender)?
There was a problem hiding this comment.
I spent over 45 minutes attempting to use createClientRender, sadly I don't have anymore time to work on this. I propose that we go with the unit tests that I've provided, and feel free to update them yourself if you'd rather use that approach.
There was a problem hiding this comment.
We are progressively moving away from enzyme, I think that any new tests using it should be a no go.
There was a problem hiding this comment.
We might look into it.
| modifiers: { | ||
| arrow: { | ||
| enabled: Boolean(arrowRef), | ||
| element: arrowRef, | ||
| }, | ||
| ...(popperOptions && popperOptions.modifiers), |
There was a problem hiding this comment.
I wouldn't if we shouldn't be using a deep merge logic (deepmerge.js), per the arrow option people might want to override: https://popper.js.org/docs/v2/modifiers/arrow/#padding (but this is v2).
There was a problem hiding this comment.
Made the switch to using deepmerge.
There was a problem hiding this comment.
I reverted the commit. deepmerge does not work with DOM elements. arrowRef was causing an infinite recursive loop. I did however add manual merging of default and custom arrow to allow for what you are asking. I do not have the time to look into refactoring deepmerge to accommodate this broken usecase. How about I make an issue to support DOM elements in deepmerge and to switch over to using it at that time?
There was a problem hiding this comment.
You didn't use the right deepmerge module. Our version should already support DOM elements.
There was a problem hiding this comment.
Can you clarify which deepmerge module we should be using (I worked with @valgrindMaster on this PR)?
We tried import deepmerge from 'deepmerge'
There was a problem hiding this comment.
oliviertassinari
added
the
PR: needs revision
oliviertassinari
changed the title
oliviertassinari
added
PR: ready to ship
and removed
PR: needs revision
|
@valgrindMaster Thanks for raising this issue and working on it! :) |
|
Thanks @oliviertassinari for helping us out with those unit tests, and for such a quick turnaround with the 4.9.5 release! Very much appreciated! |
Problem
The tooltip arrow positioning was incorrect after passing custom PopperProps to the Tooltip component. We want to pass custom Popper modifiers into the underlying popper.js instance, but could not do so without causing the tooltip arrow positioning to behave unexpectedly.
Investigation
We discovered that the arrow modifier that gets passed to popperOptions was being overwritten as a result of passing anything else in popperOptions.
Solution
Deep-merged the default popperOptions arrow modifier with the custom modifiers passed in by the user.