-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Changes from 6 commits
4e9b321
92c9bd1
c22ba7b
2bff7be
a759e1b
0496f49
da7c38c
35f5f9a
e7150a3
83419bb
e4e002b
a554065
3e8fd82
fac6c90
e4637a9
c5a9bc0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||
|
||
|
||
describe('MatProgressBar', () => { | ||
|
@@ -118,6 +120,53 @@ describe('MatProgressBar', () => { | |
expect(progressElement.componentInstance.mode).toBe('buffer'); | ||
}); | ||
}); | ||
|
||
describe('animation trigger on determinate setting', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to also have a unit test with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Or would it be something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
}); | ||
}); | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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, | ||
/** | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be single quotes There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
*/ | ||
@Output() valueAnimationEnd = new EventEmitter<number>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make the name here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can initialize this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice tip! makes code more clean. |
||
|
||
/** | ||
* Mode of the progress bar. | ||
* | ||
|
@@ -113,6 +131,21 @@ export class MatProgressBar extends _MatProgressBarMixinBase implements CanColor | |
return {transform: `scaleX(${scale})`}; | ||
} | ||
} | ||
|
||
/** Attaches value bar element animation end listener. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would remove this comment since it doesn't always describe what There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should also check the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok added, to the binding to only bind transition on _animationMode !== noop. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than only observing for animation events when it is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a couple of things:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couple questions on these before I make the change:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment can be omitted; it's pretty clear/obvious what There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. */ | ||
|
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.
Omit spaces in braces
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.
done.