-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(panel): append panel animation transforms after existing transforms #11681
fix(panel): append panel animation transforms after existing transforms #11681
Conversation
Since there is no associated issue and thus no associated demo, can you please provide a CodePen demo that reproduces the issue being fixed here? Or point to a demo on the docs site that reproduces the issue? |
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.
Can you please investigate adding some unit testing coverage for this change?
Here's a CodePen that shows the issue. Its the panel animations from the doco - but with the panel position set to .center() instead of .right().top(); |
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've tested this manually and I can see that the changes to scale
really work, but I'm not able to see any visual difference when using slide
.
Do you have some reproduction steps for me to verify the changes to slide
?
For future testing reference
|
@Noglen I just wanted to remind you that I'm still waiting on a response to my previous comment in #11681 (review). |
Sorry about that. I've had another look over it gain and can't repo the problem with Do you think I should revert back the change for I'll try and get some unit tests added in today aswell. |
Yeah, let's revert the changes to slide until we can isolate a reproduction. That should reduce the risk that this causes any issues or has problems with the presubmit process. OK, thank you for looking into adding the unit tests! |
append the transform for panel animations after any existing transforms. Previously, the animation transform was appened first, resulting in the destination location being incorrect.
d73b9e5
to
b7304e2
Compare
reverted the change to unit tests were a bit tricky since it's the browser's rending of the CSS that causes the problem with the position (since the order of operations in transform actually matters), not the actual position calc in code - so ended up just checking that the scale transform is added after the existing transform |
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.
LGTM
append the transform for panel animations after any existing transforms. Previously, the animation
transform was appened first, resulting in the destination location being incorrect.
PR Checklist
Please check that your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When a panel has a centered location (or other transform applied) combined with an OpenFrom/CloseTo, the panel animates to/from the wrong location
Issue Number:
N/A
What is the new behavior?
The panel animates to/from the correct location
Does this PR introduce a breaking change?
Other information