This repository has been archived by the owner on Dec 5, 2024. It is now read-only.
Allow popper modfiers to be updated #302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I wonder if we could add a check in developement mode to warn in case the modifiers object has the same content but changed identity? |
maclockard
commented
Sep 16, 2019
| @@ -120,15 +130,7 @@ const Demo = enhance( | |||
| ? ({ rotation, scale, opacity, top: topOffset }) => ( | |||
| <Popper | |||
| placement="bottom" | |||
| modifiers={{ | |||
There was a problem hiding this comment.
the warning allowed me to catch us over updating the modifiers object in the demo here!
maclockard
force-pushed
the
patch-1
branch
2 times, most recently
from
328d50a to
0ba3ea2
Compare
|
done! |
|
Cool thanks! I'm gonna test it out and merge it, it may take a few days unfortunately so I say sorry in advance! |
FezVrasta
reviewed
Oct 11, 2019
src/Popper.js
Outdated
| ) { | ||
|
|
||
| // develop only check that modifiers isn't being updated needlessly | ||
| if ( |
There was a problem hiding this comment.
Is Terser/Uglify/etc able to strip out this kind of condition?
Could we split it into two conditions to help the tools?
if (process.env.NODE_ENV === 'development') {
if (this.props.modifiers !== prevProps.modifiers &&
this.props.modifiers != null &&
prevProps.modifiers != null &&
shallowEqual(this.props.modifiers, prevProps.modifiers)
) {
[...]
Currently if a consumer updates the `modifiers` object prop, unless another prop also changes, the new modifiers are not applied. For example, if the passed modifier `flip.enabled` were to change from `true` to `false`, the popper doesn't pick up on that fact and continues allowing the popover contents to flip. This PR allows for the `Popper` component to construct a new popper instance with the new modifiers if and only if the `modifiers` object changes (note: this is a reference comparison and not a value comparison). I'm not that familiar with Popper.JS so if there is a more efficient way to make sure modifier changes get propagated to the underlying popover, please let me know! As it stands, this _could_ cause some unnecessary popper instance creation/destruction if the consumer isn't properly memoizing the `modifiers` object they pass. This could be partially solved by doing something like shallow comparing each modifier option object one by one instead of a top level reference comparison. Alternatively, if there is a more performant way to imperatively update an existing popper instance, that may be better to use to avoid as much churn. In fact, it would be even possible to combine both of these solutions as well.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently if a consumer updates the
modifiersobject prop, unless another prop also changes, the new modifiers are not applied. For example, if the passed modifierflip.enabledwere to change fromtruetofalse, the popper doesn't pick up on that fact and continues allowing the popover contents to flip.This PR allows for the
Poppercomponent to construct a new popper instance with the new modifiers if and only if themodifiersobject changes (note: this is a reference comparison and not a value comparison).I'm not that familiar with Popper.JS so if there is a more efficient way to make sure modifier changes get propagated to the underlying popover, please let me know!
As it stands, this could cause some unnecessary popper instance creation/destruction if the consumer isn't properly memoizing the
modifiersobject they pass. This could be partially solved by doing something like shallow comparing each modifier option object one by one instead of a top level reference comparison. Alternatively, if there is a more performant way to imperatively update an existing popper instance, that may be better to use to avoid as much churn. In fact, it would be even possible to combine both of these solutions as well.