[Popper] Fix popperOptions prop #15359
Conversation
Fixes popperjs options for onUpdate and onCreate that were being spread over in Popper component
Details of bundle changes.Comparing: 1e86a1c...3c3e56b
|
oliviertassinari
added
bug 🐛
oliviertassinari
changed the title
|
|
||
| it('should correctly pass down lifecycle callbacks to popperjs', () => { | ||
| const wrapper = mount(<Popper {...defaultProps} popperOptions={popperProps} />); | ||
| const instance = wrapper.find('Popper').instance(); |
There was a problem hiding this comment.
We will soon migrate the Popper component to the hook API. We will no longer be able to dive into the private properties of the component. The simplest option is probably not to test it.
There was a problem hiding this comment.
I also prefer tests to not call functions directly but through their public APIs i.e. In normal use
There was a problem hiding this comment.
@JaiPe Do you have a better idea than removing the test?
There was a problem hiding this comment.
I'm happy to remove it - just don't want to impact test coverage 😯! I'll have a look in the morning and see if I can test it another way, or remove the test 👍
|
@oliviertassinari Updated the test. Not sure if I'm seeing much value in it though, it feels a bit like it's just testing Popper's API. What do you think? Should I just remove it? |
|
@JaiPe It's a great first pull request on Material-UI 👌🏻. Thank you for working on it! |
* [Popper] Fix overidden popperjs lifecycle options (mui#15262) Fixes popperjs options for onUpdate and onCreate that were being spread over in Popper component * fix formatting * update test
Fixes popperjs options for onUpdate and onCreate that were being spread over in Popper component
Closes #15262