pass whole popperConfig to PopperJS (#748) #750
Conversation
src/usePopper.js
Outdated
| }; | ||
|
|
||
| popperInstanceRef.current = new PopperJS(referenceElement, popperElement, { | ||
| ...options, |
There was a problem hiding this comment.
should this use an options.popperConfig? otherwise we'd need to spread out the properties under options that we consume above.
probably doing an options.popperConfig makes the typing easier, too
There was a problem hiding this comment.
options.placement, options.modifiers, options.eventsEnabled is separate props, options.popperConfig is composite property of the remaining options of Popperjs. I think this could confuse users.
There was a problem hiding this comment.
This is how e.g. <Overlay> works, though, no?
There was a problem hiding this comment.
shoot, my bad, that's not how <Overlay> works. it does spread in the popper config like you had here. sorry about that.
but if i'm not mistaken, then, i think in usePopper, ...popperConfig should just be the options not explicitly consumed?
There was a problem hiding this comment.
how Overlay works
I didn't notice it.
There was a problem hiding this comment.
no, sorry, i meant your initial implementation was correct. my suggestion above was bad. we should spread in ...popperOptions into usePopper. we just need to also spread out the options that we explicitly consume there. otherwise it would be a breaking change
* use options.popperConfig to initiate PopperJS
* fixed pass popperConfig option
This reverts commit 453ff44
|
@jquense ping |
|
@Viktor-the-great sorry for the delay |
3 participants
No description provided.