-
Notifications
You must be signed in to change notification settings - Fork 20
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
Remove scale animation from route transitions #173
Remove scale animation from route transitions #173
Conversation
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.
While this does remove the scale animation, it also removes the transitions and timing we typically use. We want to keep those.
Can we also get a better PR description here? If we are making these changes on purpose we need to know the reasons behind them.
@@ -20,16 +18,7 @@ export const routerAnimation = | |||
|
|||
query(':leave', | |||
[ | |||
style({ opacity: 1 }), | |||
animate(timing + easing, style({ opacity: 0, transform: 'scale(0.95)' })) |
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.
What was the reasoning for removing the transition here?
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.
style({ opacity: 1 })
is still apart of the :leave
transition (just the diff is making it look weird). As for the animation, I didn't see a benefit leaving it in without the scale. I'll put them back in without the scale and see I could see a difference 😄
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 added back the animations
@AlexOverbeck I figured linking the issue would be a good enough description as it defines purpose of the change. I can always copy what it's in the issue as not sure what more I can add. |
@patryk0605, it makes sense to add the issue number. This is a pretty simple change so I understand the brevity. We will likely be adding a pull request template soon to give a little more detail around what we will be looking for in the future but for now this is okay. The changes here look good, but it seems like there was a bad rebase. Could we please fix up this history? |
@AlexOverbeck thanks for the feedback about the PR description. I will try to be more descriptive going forward until the template is released. And sorry about that ugly commit history 😄 - all fixed up now sir! |
Thanks for your work on this one! 🎸 |
Per issue #159, scale animation is removed as it was deemed too abrasive.