Skip to content

feat(animation): support animating width/height properties (WIP) #4917

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

Merged

Conversation

justindujardin
Copy link
Contributor

@justindujardin justindujardin commented Oct 4, 2017

Add support for animating width/height View properties.

Implements #1764

  • Supports animating PercentageLength properties
  • Works with Angular component animations
  • Works on Android
  • Works on iOS
  • Add example to examples app
    • basic height animation
    • height animation in stack layout
    • animation curves tester
    • details disclosure effect
  • Verify works as expected with "auto" height
  • Verify works with all built-in animation curves
  • Verify keyframe animation support
  • Add unit tests
    • PercentLength.parse tests to verify input/output expectations
    • View.animate resolves percentage width/height relative to parent
    • View.animate works with pixel value strings
    • View.animate works with number dip inputs
    • View.animate works with number dip inputs
    • Keyframe animation tests
    • Spring curve tests (iOS has separate codepaths for spring)

Use with View.animate

view.animate({
  width:'40%', 
  height: '400px',
  duration: 300
});

Use with Angular

@Component({
  moduleId: module.id,
  selector: 'cool-component',
  template: `
      <Button (tap)="details = !details" text="Toggle"></Button>
      <Image backgroundColor="blue" height="100%"
             [@sizingImage]="details == true ? 'details' : 'summary'"></Image>
  `,
  animations: [
    trigger('sizingImage', [
      state('details', style({height: '55%'})),
      state('summary', style({height: '100%'})),
      transition('summary => details', animate('200ms ease-out')),
      transition('details => summary', animate('200ms ease-in')),
    ]),
  ]
})
export class CoolComponent {
  details = true;
}

Unfinished thoughts

There are a few limitations that could be lifted, but need input:

  • Layout values are not interpolated for Views on iOS. StackLayout height animations will "jump" a little. Would need to add some kind of periodic JS callback during the animation to do this.
  • View originX/Y values are different on iOS/Android by default. This means that height/width animations will act differently by default if the user does not specify origins explicitly on the View being animated.

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

13 similar comments
@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Oct 4, 2017

Can one of the admins verify this patch?

@ghost ghost added the ♥ community PR label Oct 4, 2017
@ns-bot ns-bot added the cla: yes label Oct 4, 2017
 - width/height can be specified in any valid PercentLength form that can be parsed.
 - make width/height properties be based on animatable CSS property. TODO: affectsLayout????
 - add a few basic tests. Could probably use a few more?
 - fix a few null pointer exceptions in PercentLength helpers
@justindujardin justindujardin force-pushed the animate-width-height branch 2 times, most recently from 4845d64 to 05e021a Compare October 5, 2017 23:13
 - basic height animation
 - height animation in StackLayout
 - fix an issue where strings were not automatically converted to PercentLength when calling directly into `View.animate`
@justindujardin
Copy link
Contributor Author

justindujardin commented Oct 6, 2017

I added a few UI tests. With StackLayout there is a difference between how the animation looks on iOS and Android.

Android

A time update function is required to animate height on a view, so the layout updates smoothly as you would expect.

https://user-images.githubusercontent.com/101493/31256882-fd9e9e8e-a9e9-11e7-9b67-51c7d09bc905.gif

iOS

There is no time update callback in Javascript, so the value isn't animated as far as the {N} Layout is concerned.

https://user-images.githubusercontent.com/101493/31256879-f80ea4a0-a9e9-11e7-8021-b35ed081746b.gif

What to do?

It looks like there are some options to add a javascript callback that could update the Layout value over time on iOS, but it would almost certainly slow things down and add complexity.

It's not clear to me if it's worth slowing down all height animations to satisfy a particular layout constraint. e.g. this poor animation would have to pay for a feature it doesn't need. 😭

https://user-images.githubusercontent.com/101493/31257754-51e1a37e-a9ef-11e7-9815-8cb5fbc31a0a.gif

Thoughts?

 - use height transition to cover textview content.
 - when clicking on the summary view, animate the summary height up to a small header and show the text view.
 - fake animating the height on the textview by very subtly animating its translateY value while shrinking the header height. This tricks your mind into think that the text view is also vertically growing, even thought it's just slightly moving up along the Y axis.
@justindujardin justindujardin force-pushed the animate-width-height branch 2 times, most recently from d8a5d30 to b2b38e4 Compare October 6, 2017 21:51
 - verify all built-in animation curve types work as expected.
@justindujardin
Copy link
Contributor Author

justindujardin commented Oct 6, 2017

Added a few more examples to test corner cases:

Scrolling text view with animated header

https://user-images.githubusercontent.com/101493/31301337-0ac163fc-aaae-11e7-817d-4833c95a9cca.gif

Animation curves

https://user-images.githubusercontent.com/101493/31301334-040b38a8-aaae-11e7-9853-9ada0fa49a63.gif

@justindujardin justindujardin changed the title feat(animation): support animating width/height properties feat(animation): support animating width/height properties (WIP) Oct 14, 2017
@justindujardin
Copy link
Contributor Author

I missed testing a few things, so I put this back in WIP, but I still welcome reviews/feedback.

@SvetoslavTsenov
Copy link
Contributor

Hey @justindujardin, thank you for your contribution. We are going to execute some tests and review the PR.

@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@NativeScript NativeScript deleted a comment from ns-bot Dec 4, 2017
@SvetoslavTsenov SvetoslavTsenov merged commit 57ed0cf into NativeScript:master Dec 5, 2017
@jogboms
Copy link

jogboms commented Dec 5, 2017

@justindujardin 🥇🥇🥇

@justindujardin justindujardin deleted the animate-width-height branch December 5, 2017 17:28
vchimev pushed a commit that referenced this pull request Dec 6, 2017
vchimev pushed a commit that referenced this pull request Dec 7, 2017
vchimev pushed a commit that referenced this pull request Dec 7, 2017
vchimev pushed a commit that referenced this pull request Dec 7, 2017
@davecoffin
Copy link
Contributor

is this scheduled for release?

@RoyiNamir
Copy link

How can I know if this already available and on which version?

@vchimev
Copy link
Contributor

vchimev commented Mar 16, 2018

Hello All,

Great work on this pull-request! You can follow the progress on it here.

Our main concerns with this implementation are related to performance. I am afraid that we have not had the time to measure and investigate it due to higher priority tasks. However, we are aware of its importance and wide demand, that is why we would like to invest further efforts and improve this feature as well as the animations at all.

This has not been scheduled for release yet. Currently, we are working on the 4.1 roadmap, so I hope we could get it there. Please follow the issue and this pull-request for any comments and updates.

MartoYankov pushed a commit that referenced this pull request May 7, 2019
VladimirAmiorkov pushed a commit that referenced this pull request Jun 6, 2019
@lock
Copy link

lock bot commented Aug 26, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants