This repository has been archived by the owner. It is now read-only.

docs(animate): add animations documentation #1491

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@teropa
Contributor

teropa commented May 23, 2016

No description provided.

@googlebot googlebot added the CLA: yes label May 23, 2016

@teropa

This comment has been minimized.

Contributor

teropa commented May 23, 2016

@matsko, @wardbell animations guide ready for your review.

Not ready to merge yet. Need to release ng-animate itself first. 😄 Also needs E2E tests.

animations: [
animation('heroState', [
// Styles in each state
state('void', style({})),

This comment has been minimized.

@teropa

teropa May 23, 2016

Contributor

@matsko The explicit void state seems to be necessary because there’s an exception on the initial void => inactive transition otherwise. Is it so that we need to have something for all states including void, even if we don’t have a transition for it?

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

You can leave out a state declaration and Angular won't throw an error. Go ahead and remove this line since there are no styles set for the void state.

img(src="/resources/images/devguide/animations/ng_animate_transitions_inactive_active.png" alt="In Angular animations we defines states and transitions between states" width="400")
:marked
### The wildcard state `*`

This comment has been minimized.

@teropa

teropa May 23, 2016

Contributor

Is "wildcard" what we should call it?

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

wildcard is fine

style({height: '*'}),
animate(250, style({height: 0}))
])
])

This comment has been minimized.

@teropa

teropa May 23, 2016

Contributor

@matsko Should we also be able to do a similar thing for entering? I tried it but couldn't figure out how to make it so that it has a usable computed height in void => *

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

height: '*' should be fine since it gets removed when the animation completes.

animate('100ms ease-out', style(':inactive'))
])
])
]

This comment has been minimized.

@teropa

teropa May 23, 2016

Contributor

@matsko I also tried an alternative where the styles are not explicitly referred to from the transition, but was not able to make it work:

animation('heroState', [
   state('inactive', style({backgroundColor: '#eee', transform: 'scale(1)'})),
   state('active',   style({backgroundColor: 'lightblue', transform: 'scale(1.1)'})),
   transition('inactive => active', animate('100ms ease-in’)),
   transition('active => inactive', animate('100ms ease-out’))
])

Should this kind of thing be supported? This is our first "quick start" example, so I'm trying to make it have as few moving parts as possible.

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

This should work fine now.

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

You can also use one transition as both if the animate call is the same.

// <=>
transition('inactive <=> active', animate('100ms ease-in’))

// comma separated
transition('inactive => active, active => inactive', animate('100ms ease-in’))
styleUrls: ['hero-list.component.css'],
// #docregion animationdef
animations: [
animation('shrinkOut', [

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

This got renamed to trigger(

animate('100ms ease-in', style(':active'))
]),
transition('active => inactive', [
style(':active'),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

This gets applied automatically now. There is no need to reference the starting state.

state('inactive', style('.inactive')),
state('active', style('.active')),
transition('inactive => active', [
style(':inactive'),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

Remove

animate(100, style(':active'))
]),
transition('active => inactive', [
style(':active'),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

Remove

})),
// Transitions between states
transition('inactive => active', [
style(':inactive'),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

Remove

state('inactive', style({transform: 'translateX(0) scale(1)'})),
state('active', style({transform: 'translateX(0) scale(1.1)'})),
transition('inactive => active', [
style(':inactive'),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

Remove

animate('100ms ease-in', style(':active'))
]),
transition('active => inactive', [
style(':active'),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

Remove

animate(200, style(':active'))
]),
transition('active => void', [
style(':active'),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

Remove

animate(100, style(':inactive'))
]),
transition('inactive => void', [
style(':inactive'),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

Remove

]),
transition('void => active', [
style({transform: 'translateX(0) scale(0)'}),
animate(200, style(':active'))

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

No need to include the style portion

]),
transition('void => inactive', [
style({transform: 'translateX(-100%) scale(1)'}),
animate(100, style(':inactive'))

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

No need to include the style portion

animations: [
animation('flyInOut', [
transition('void => *', [
animate(300, style('@flyIn'))

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

This doesn't work just yet. The CSS parser needs to be included into Angular first. The syntax will also change to be:

animate(300, keyframes('flyIn'))
// #docregion animationdef
animations: [
animation('heroState', [
state('inactive', style('.inactive')),

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

These are not ready to use yet since the CSS parser has not been integrated into animations just yet.

img(src="/resources/images/devguide/animations/ng_animate_transitions_void_in.png" alt="The void state can be used for enter and leave transitions" width="400")
:marked
The wildcard state `*` does not match `void`. We can think of `*` as "anything but `void`".

This comment has been minimized.

@matsko

matsko May 25, 2016

Member

* also matches void

@teropa

This comment has been minimized.

Contributor

teropa commented May 26, 2016

@matsko Thanks for the comments! Guide updated accordingly.

// #docregion animationdef
animations: [
trigger('shrinkOut', [
state('in', style({height: '*'})),

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

You should document what is happening here on L30-L34... comments are sufficient IMO

template: `
<ul>
<li *ngFor="let hero of heroes.get()"
@heroState="hero.state"

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

Recommend you call this @heroSelected since this reflects (via styles) whether the hero is active/inactive (based on click selections).

Also recommend some comments above the (click):

// Clicks on a 'hero' row will activate/select the current row and deselect all other rows
// This list will animation the styles based on 'selected' state changes on the hero row.
@@ -0,0 +1,39 @@
import {

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

Should comment how this example is different than the others.
e.g.

/**
 * This 'hero-list' example uses CSS classNames with state changes to animate 
 * 'hero'  row selections
 */
@@ -0,0 +1,53 @@
// #docregion

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

Should comment how this example is different than the others.
e.g.

/**
 * This 'hero-list' example uses inline CSS styles with state changes to animate 
 * 'hero'  row selections. Clicks on a 'hero' row will activate/select the current row 
 * and deselect all other rows. This list will animation the styles based on 'selected' 
 * state changes on the hero row.
 */
style({transform: 'translateX(-100%)'}),
animate(100)
]),
transition('* => void', [

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

Comment here that we are explicitly defining transitions for

  • in using the void => * and
  • out using * => void
state('in', style({width: 120, transform: 'translateX(0)', opacity: 1})),
transition('void => *', [
style({width: 10, transform: 'translateX(50px)', opacity: 0}),
group([

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

Comment here that the group runs each item in parallel

@@ -0,0 +1,50 @@
import {

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

Do we have a Table of Contents for these examples with titles, purpose and differences columns ?

This comment has been minimized.

@teropa

teropa May 30, 2016

Contributor

The examples are shown at different points in the guide (upgrade.jade) and explained in context there. Beyond that (for Plunker users basically), there's currently only the column titles in hero-team-builder.component.ts

transform: 'scale(1.1)'
})),
// #docregion transitions
transition('inactive <=> active', animate('100ms ease-out'))

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

Comment here that transitions supports two-way state changes in a single definition.

public state = 'inactive') {
}
toggleActivation() {

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

In the ^^ animations, I thought the toggleActivation() was exclusively so only a single items is active (hence the name activation). But you are not notifying the heroList to deselect any other active items.

So I wonder if activation is the best wording here.

currentHeroes = [];
get() {

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

I recommend using an Iterator interface pattern here with hasNext(), next(), etc.

template: `
<button [disabled]="!heroes.canAdd()" (click)="heroes.add()">Add</button>
<button [disabled]="!heroes.canRemove()" (click)="heroes.remove()">Remove</button>
<div class="columns">

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

See earlier comment about TOC to provide more information. Perhaps a column mouseOver to show details in the footer pane ?

A more lightweight polyfill maintained by the Angular team is coming soon.
:marked
# Table of Contents

This comment has been minimized.

@ThomasBurleson

ThomasBurleson May 30, 2016

Contributor

Something like this would be great as part of the the Hero HTML; perhaps using mouseOver effects.

@ThomasBurleson

This comment has been minimized.

Contributor

ThomasBurleson commented May 30, 2016

@teropa: I am very impressed with this PR... great work!

  • Love the detail with all the examples
  • Love the animated gifs
  • Love the transitions PNG diagrams
@teropa

This comment has been minimized.

Contributor

teropa commented May 30, 2016

@ThomasBurleson Thank you for the review! I will dig into your comments the first chance I get.

@ThomasBurleson

This comment has been minimized.

Contributor

ThomasBurleson commented May 30, 2016

@teropa - Most of my feedback is around commenting the actual source: so developers can read the meaning [in the source itself] about some of the more subtle nuances.

@Foxandxss Foxandxss changed the title from WIP docs(animate): add animations documentation to docs(animate): add animations documentation Jun 9, 2016

@googlebot

This comment has been minimized.

googlebot commented Jun 14, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

1 similar comment
@googlebot

This comment has been minimized.

googlebot commented Jun 14, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added CLA: no and removed CLA: yes labels Jun 14, 2016

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Jun 14, 2016

Hush googlebot, Tero and Jesus agree on those commits.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Jun 14, 2016

I made the code lintable (I had to ignore one file from the gulpfile until mgechev/codelyzer#60 is fixed.

I also removed (saved it locally tho) the future features of animation per petition of @matsko .

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Jun 15, 2016

On master

@Foxandxss Foxandxss closed this Jun 15, 2016

@naomiblack

This comment has been minimized.

Member

naomiblack commented Jun 15, 2016

and live!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.