feat(sort): add sorting indicator animation #5831
Conversation
googlebot
added
the
cla: yes
|
@andrewseguin can you check with the Material Design UX team whether the sort header should have an animation and what that animation should be? |
|
Checked with the spec, it should rotate when the sort changes. Will check out the PR soon |
src/lib/sort/sort-header.html
Outdated
| @@ -7,8 +7,7 @@ | |||
|
|
|||
| <div *ngIf="_isSorted()" | |||
| class="mat-sort-header-arrow" | |||
| [class.mat-sort-header-asc]="_sort.direction == 'asc'" | |||
| [class.mat-sort-header-desc]="_sort.direction == 'desc'"> | |||
| [@indicatorRotate]="_getSortingState()"> | |||
There was a problem hiding this comment.
Can just use _sort.direction without needing a function
src/lib/sort/sort-header.ts
Outdated
| trigger('indicatorRotate', [ | ||
| state('asc', style({transform: 'rotate(45deg)'})), | ||
| state('desc', style({transform: 'rotate(225deg)'})), | ||
| /** Use standart animation curve from material guidelines */ |
src/lib/sort/sort-header.ts
Outdated
| state('asc', style({transform: 'rotate(45deg)'})), | ||
| state('desc', style({transform: 'rotate(225deg)'})), | ||
| /** Use standart animation curve from material guidelines */ | ||
| transition('asc <=> desc', animate('225ms cubic-bezier(0.4,0.0,0.2,1)')) |
There was a problem hiding this comment.
Needs spaces between the numbers: cubic-bezier(0.4, 0.0, 0.2, 1);
|
Looks good, just a few comments. Thanks for the PR - animations look good |
andrewseguin
added
pr: lgtm
action: merge
|
@KillaBee1985 can you squash this down into a single commit? Our presubmit script doesn't like the merge commits |
src/lib/sort/sort-header.html
Outdated
| @@ -7,8 +7,7 @@ | |||
|
|
|||
| <div *ngIf="_isSorted()" | |||
| class="mat-sort-header-arrow" | |||
| [class.mat-sort-header-asc]="_sort.direction == 'asc'" | |||
| [class.mat-sort-header-desc]="_sort.direction == 'desc'"> | |||
| [@indicatorRotate]="this._sort.direction"> | |||
There was a problem hiding this comment.
Although the use of this in template is valid (until now), it may be broken in future angular versions, since it's not even documented. Btw, I could not find the question I had seen in SO that discussed this better.
There was a problem hiding this comment.
Just find this: https://stackoverflow.com/q/44667219/4911842
|
@mmalerba, I removed all merges and left only my commits. Is it ok? |
|
yep, that works too! |
|
Please rebase |
|
@KillaBee1985 can you rebase this? So we can get this merged. |
Uses @angular/animations module to animate sort order change.
|
@rafaelss95 Done. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Uses @angular/animations module to animate sort order change.