Skip to content

Add CSS Transition end output on mat-progress-bar value animation #12409

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
merged 16 commits into from
Aug 28, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/demo-app/progress-bar/progress-bar-demo.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ <h1>Determinate</h1>

<div class="demo-progress-bar-controls">
<span>Value: {{determinateProgressValue}}</span>
<br/>
<span>Last animation complete value: {{animationEndValue}}</span>
<button mat-raised-button (click)="stepDeterminateProgressVal(10)">Increase</button>
<button mat-raised-button (click)="stepDeterminateProgressVal(-10)">Decrease</button>
</div>

<div class="demo-progress-bar-container">
<mat-progress-bar mode="determinate" [value]="determinateProgressValue" [color]="color">
<mat-progress-bar mode="determinate" [value]="determinateProgressValue" [color]="color" (valueAnimationEnd)="logValue($event)">
</mat-progress-bar>
</div>

Expand Down
5 changes: 5 additions & 0 deletions src/demo-app/progress-bar/progress-bar-demo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {Component} from '@angular/core';
export class ProgressBarDemo {
color: string = 'primary';
determinateProgressValue: number = 30;
animationEndValue: number;
bufferProgressValue: number = 30;
bufferBufferValue: number = 40;

Expand All @@ -35,6 +36,10 @@ export class ProgressBarDemo {
this.bufferBufferValue = this.clampValue(val + this.bufferBufferValue);
}

logValue(val: number) {
this.animationEndValue = val;
}

private clampValue(value: number) {
return Math.max(0, Math.min(100, value));
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/progress-bar/progress-bar.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@
<rect [attr.fill]="_rectangleFillValue" width="100%" height="100%"/>
</svg>
<div class="mat-progress-bar-buffer mat-progress-bar-element" [ngStyle]="_bufferTransform()"></div>
<div class="mat-progress-bar-primary mat-progress-bar-fill mat-progress-bar-element" [ngStyle]="_primaryTransform()"></div>
<div class="mat-progress-bar-primary mat-progress-bar-fill mat-progress-bar-element" [ngStyle]="_primaryTransform()" #primaryValueBar></div>
<div class="mat-progress-bar-secondary mat-progress-bar-fill mat-progress-bar-element"></div>
53 changes: 51 additions & 2 deletions src/lib/progress-bar/progress-bar.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import {TestBed, async, ComponentFixture} from '@angular/core/testing';
import {Component} from '@angular/core';
import {TestBed, async, ComponentFixture, fakeAsync, tick} from '@angular/core/testing';
import {Component, DebugElement} from '@angular/core';
import {By} from '@angular/platform-browser';
import {dispatchFakeEvent} from '@angular/cdk/testing';
import {Location} from '@angular/common';
import {MatProgressBarModule} from './index';
import { MatProgressBar } from './progress-bar';
Copy link
Member

Choose a reason for hiding this comment

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

Omit spaces in braces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.



describe('MatProgressBar', () => {
Expand Down Expand Up @@ -118,6 +120,53 @@ describe('MatProgressBar', () => {
expect(progressElement.componentInstance.mode).toBe('buffer');
});
});

describe('animation trigger on determinate setting', () => {
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to also have a unit test with NoopAnimationsModule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not completely following the test where NoopAnimationsModule here, would it look like:

  1. Check that event is not bound?
  2. On animation === noop, always call output on value change?

Or would it be something else?

Copy link
Member

Choose a reason for hiding this comment

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

I want to ensure that we have a test that covers the code path where NoopAnimationsModule is loaded and someone tries to register this event. The goal being to ensure that no error is thrown more so than that a particular outcome occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks that makes sense. I added some more test with NoopAnimations module turned on. Also I make the animationEnd fire when setting the value in NoopAnimations mode, my thoughts is that page author might be relying on this output to update their DOM in their test so we should fire the output synchronously when there's no animation. PTAL.

let fixture: ComponentFixture<BasicProgressBar>;
let progressComponent: MatProgressBar;
let primaryValueBar: DebugElement;

beforeEach(() => {
fixture = TestBed.createComponent(BasicProgressBar);
fixture.detectChanges();

const progressElement = fixture.debugElement.query(By.css('mat-progress-bar'));
progressComponent = progressElement.componentInstance;
primaryValueBar = progressElement.query(By.css('.mat-progress-bar-primary'));
});

it('should trigger output event on primary value bar animation end', () => {
spyOn(progressComponent.valueAnimationEnd, "next");

progressComponent.value = 40;
dispatchFakeEvent(primaryValueBar.nativeElement, 'transitionend');
expect(progressComponent.valueAnimationEnd.next).toHaveBeenCalledWith(40);
});
});

describe('animation trigger on buffer setting', () => {
let fixture: ComponentFixture<BufferProgressBar>;
let progressComponent: MatProgressBar;
let primaryValueBar: DebugElement;

beforeEach(() => {
fixture = TestBed.createComponent(BufferProgressBar);
fixture.detectChanges();

const progressElement = fixture.debugElement.query(By.css('mat-progress-bar'));
progressComponent = progressElement.componentInstance;
primaryValueBar = progressElement.query(By.css('.mat-progress-bar-primary'));
});

it('should trigger output event with value not bufferValue', () => {
spyOn(progressComponent.valueAnimationEnd, "next");

progressComponent.value = 40;
progressComponent.bufferValue = 70;
dispatchFakeEvent(primaryValueBar.nativeElement, 'transitionend');
expect(progressComponent.valueAnimationEnd.next).toHaveBeenCalledWith(40);
});
});
});


Expand Down
37 changes: 35 additions & 2 deletions src/lib/progress-bar/progress-bar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,16 @@ import {
ElementRef,
Inject,
Input,
Output,
EventEmitter,
Optional,
ViewEncapsulation
ViewEncapsulation,
AfterViewInit,
ViewChild,
OnDestroy
} from '@angular/core';
import {Location} from '@angular/common';
import {fromEvent, Subscription} from 'rxjs';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
import {CanColor, mixinColor} from '@angular/material/core';

Expand Down Expand Up @@ -54,7 +60,7 @@ let progressbarId = 0;
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
})
export class MatProgressBar extends _MatProgressBarMixinBase implements CanColor {
export class MatProgressBar extends _MatProgressBarMixinBase implements CanColor, AfterViewInit, OnDestroy {
constructor(public _elementRef: ElementRef,
@Optional() @Inject(ANIMATION_MODULE_TYPE) public _animationMode?: string,
/**
Expand Down Expand Up @@ -82,6 +88,18 @@ export class MatProgressBar extends _MatProgressBarMixinBase implements CanColor
set bufferValue(v: number) { this._bufferValue = clamp(v || 0); }
private _bufferValue: number = 0;

@ViewChild("primaryValueBar") primaryValueBar: ElementRef;
Copy link
Member

Choose a reason for hiding this comment

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

This should be single quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


/**
* Event that indicates animation end on value bar. Currently only support determinate and buffer.
* Also note, if multiple event update is thrown in quick succession the last event completion is
* the one firing the output.
Copy link
Member

Choose a reason for hiding this comment

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

I would rephrase the comment to say that the event will never be emitted for animations that don't "end" (i.e. 'query' and 'indeterminate'). Also that the event will never fire if animations are disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

*/
@Output() valueAnimationEnd = new EventEmitter<number>();
Copy link
Member

Choose a reason for hiding this comment

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

I would make the name here animationEnd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

We generally avoid events that emit a primitive value because you can't ever add additional fields later. Instead an interface is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created event interface, looks more flexible now, thanks!


/** Reference to animation end subscription to be unsubscribed on destroy. */
private _valueAnimationSubscription: Subscription | null;
Copy link
Member

Choose a reason for hiding this comment

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

You can initialize this to Subscription.EMPTY. That way you won't have to null check it everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice tip! makes code more clean.


/**
* Mode of the progress bar.
*
Expand Down Expand Up @@ -113,6 +131,21 @@ export class MatProgressBar extends _MatProgressBarMixinBase implements CanColor
return {transform: `scaleX(${scale})`};
}
}

/** Attaches value bar element animation end listener. */
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this comment since it doesn't always describe what ngAfterViewInit does. Really the best thing would be to add another method like _addAnimationEndListener and call it here, but I don't feel that strongly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the comment inside the callback hopefully that helps with documentation.

ngAfterViewInit() {
if(this.mode === 'determinate' || this.mode === 'buffer') {
Copy link
Member

Choose a reason for hiding this comment

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

Space after if (here and below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

This should also check the _animationMode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok added, to the binding to only bind transition on _animationMode !== noop.

Copy link
Member

Choose a reason for hiding this comment

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

Rather than only observing for animation events when it is determinate or buffer, we should always set up the subscription and only emit out valueAnimationEnd event when appropriate. Otherwise, the mode of the MatProgressBar cannot be changed dynamically and still utilize this event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's nice tip! Definitely more flexible that way, I guess one possible downside is that the output is always executed on unsupported method, but that seems like small overhead?

this._valueAnimationSubscription = fromEvent(this.primaryValueBar.nativeElement, 'transitionend')
Copy link
Member

Choose a reason for hiding this comment

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

This needs a couple of things:

  1. Since the transitionend event bubbles, it means that it could fire if a transition completes for an element inside the track. We should filter out based on whether event.target === this.primaryValueBar.nativeElement.
  2. The event should be bound outside the NgZone and it should re-enter the zone only when it emits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couple questions on these before I make the change:

  1. The event is bound to specifically div mat-progress-bar-primary element inside the template, so adding the filter target would be redundant? What am I missing here?
  2. Is this more of best practice to run CSS event on NgZone? Would re-enter zone looks like this? https://github.com/angular/material2/blob/c2fc3f45b5ccdbff3ade56e0685b621688e9cb6d/src/cdk/text-field/autofill.ts#L72.

Copy link
Member

Choose a reason for hiding this comment

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

  1. That's true, but it's more of a future-proofing thing in case we decide to put something into the element.
  2. It's only really necessary if we end up doing point 1, otherwise it would be redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks for the explanation. I've updated the latest commit with these two points incorporated. PTAL.

.subscribe(() => this.valueAnimationEnd.next(this.value));
}
}

/** Removes value bar element animation end listener. */
Copy link
Member

Choose a reason for hiding this comment

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

This comment can be omitted; it's pretty clear/obvious what ngOnDestroy is doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

ngOnDestroy() {
if(this._valueAnimationSubscription) {
this._valueAnimationSubscription.unsubscribe();
}
}
}

/** Clamps a value to be between two numbers, by default 0 and 100. */
Expand Down