Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(panel): append panel animation transforms after existing transforms #11681

Merged
merged 1 commit into from
Jun 17, 2019

Conversation

Noglen
Copy link
Contributor

@Noglen Noglen commented Mar 15, 2019

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:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

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?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Mar 15, 2019
@Splaktar Splaktar self-requested a review April 1, 2019 03:24
@Splaktar Splaktar self-assigned this Apr 1, 2019
@Splaktar Splaktar added needs: demo A CodePen demo or GitHub repository is needed to demonstrate the reproduction of the issue needs: unit tests This PR needs unit tests to cover the changes being proposed labels Apr 1, 2019
@Splaktar
Copy link
Member

Splaktar commented Apr 1, 2019

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?

Copy link
Member

@Splaktar Splaktar left a 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?

@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Apr 1, 2019
@Noglen
Copy link
Contributor Author

Noglen commented Apr 1, 2019

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();

https://codepen.io/noglen/pen/qwBMOx

@Splaktar Splaktar removed the needs: demo A CodePen demo or GitHub repository is needed to demonstrate the reproduction of the issue label Apr 1, 2019
Copy link
Member

@Splaktar Splaktar left a 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?

@Splaktar Splaktar added this to the 1.1.19 milestone Apr 1, 2019
@Splaktar Splaktar added hotlist: animations P4: minor Minor issues. May not be fixed without community contributions. labels Apr 1, 2019
@Splaktar
Copy link
Member

Splaktar commented Apr 1, 2019

For future testing reference

@Splaktar Splaktar added the needs: feedback The issue creator or community need to respond to questions in this issue label May 30, 2019
@Splaktar
Copy link
Member

@Noglen I just wanted to remind you that I'm still waiting on a response to my previous comment in #11681 (review).

@Splaktar Splaktar modified the milestones: 1.1.19, 1.1.20 May 30, 2019
@Noglen
Copy link
Contributor Author

Noglen commented May 30, 2019

@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 slide, so i'm gussing it was something else I had in my project that was causing the issue.

Do you think I should revert back the change for slide, or leave the change so that it is consistant with scale?

I'll try and get some unit tests added in today aswell.

@Splaktar
Copy link
Member

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.
@Noglen Noglen force-pushed the wip/fix-panel-animation-transform branch from d73b9e5 to b7304e2 Compare May 31, 2019 05:54
@Noglen
Copy link
Contributor Author

Noglen commented May 31, 2019

reverted the change to slide

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

@Splaktar Splaktar removed needs: feedback The issue creator or community need to respond to questions in this issue in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: unit tests This PR needs unit tests to cover the changes being proposed labels May 31, 2019
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Splaktar Splaktar added the pr: lgtm This PR has been approved by the reviewer label May 31, 2019
@Splaktar Splaktar added the pr: merge ready This PR is ready for a caretaker to review label May 31, 2019
@Splaktar Splaktar assigned mmalerba and unassigned josephperrott Jun 13, 2019
@mmalerba mmalerba merged commit ffe9349 into angular:master Jun 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ hotlist: animations P4: minor Minor issues. May not be fixed without community contributions. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants