-
Notifications
You must be signed in to change notification settings - Fork 739
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
Fix layout animations for elements that only change layout relative to parent #1271
Fix layout animations for elements that only change layout relative to parent #1271
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 77e187c:
|
right: 390, | ||
top: 160, | ||
}) | ||
// matchViewportBox(newBox, { |
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 are we skipping this test? Is it no longer the expected behaviour?
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.
Ooooops thanks
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.
Please still pass
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.
NOOOO
@@ -807,7 +814,11 @@ export function createProjectionNode<I>({ | |||
* a relativeParent. This will allow a component to perform scale correction | |||
* even if no animation has started. | |||
*/ | |||
if (!this.targetDelta && !this.relativeTarget) { | |||
if ( | |||
this.safeToResolveRelativeTarget && |
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 do we need this check? After the first time it should get a relative target right? 🤔
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 yeah I've removed it, I think this is not needed any more
@@ -893,7 +907,10 @@ export function createProjectionNode<I>({ | |||
if (!this.parent || hasTransform(this.parent.latestValues)) | |||
return undefined | |||
|
|||
if (this.parent.target && this.parent.layout) { | |||
if ( | |||
(this.parent.target || this.parent.targetDelta) && |
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.
Should we just check targetDelta
? When do we expecting target
to be there but not targetDelta
?
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 think maybe if we check for relativeTarget instead it'll catch both but earlier in the frame
b2dc4ae
to
77e187c
Compare
Previously we weren't properly animating elements that DON'T change layout relative to the page/viewport but DO change layout relative to the parent.