-
-
Notifications
You must be signed in to change notification settings - Fork 870
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.
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
Ensure optimised appear animations are cancelled when styles differ #2772
Conversation
29bf229
to
12a03d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some questions but looks good overall.
): string | undefined { | ||
return visualElement.getProps()[optimizedAppearDataAttribute] | ||
return visualElement.props[optimizedAppearDataAttribute] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Practically it doesn't make a difference but it just made the types subset of VisualElement
simpler
* instance if the element's appear animation is on a different | ||
* breakpoint. | ||
*/ | ||
if (!valueName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what cases do we not provide a value name for the optimized animation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this instance it will check for any optimised animation - for instance when determining if we fire animations on layout effect or effect
if (!this.valueSubscriptions.has(key)) { | ||
this.bindToMotionValue(key, value) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't fully understand this change, as bindToMotionValue
seems to be designed to handle cases where a value subscription is present as well. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch - this is a legacy from fixing the same bug that other check fixed (that I discovered while making this PR)
appearId, | ||
valueName | ||
) | ||
removeSyncCheck() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with the value.on
API, why do we need to call removeSyncCheck
again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think of it like
function doThing() {
if (someCondition) {
// stop doing thing
element.removeEventListener("mousestart", doThing)
}
}
element.addEventListener("mousestart", doThing)
if (Math.round(left) !== 300) { | ||
showError( | ||
box, | ||
`optimised animation not cancelled by external value mismatch with rendered style` | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opacity animation should not be canceled, right? Do we also want to check that as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll add a check - it should be cancelled by mismatches as well
In Framer we provided two
MotionValue
s for each component:values
, this value handlesanimate
/whileHover
etc animationsstyle
, this value is renderedThe value provided to
style
is composed from otherMotionValue
s each handling an effect, for instance scroll transform etc.If this rendered value falls out of sync with the one being used by Motion for its internal animations, then we're in a situation where the optimised appear animation must also have fallen out of sync and we need to cancel it. This PR implements this.