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

glouischandra · 2018-07-27T19:06:17Z

Demo of the issue: https://stackblitz.com/edit/matprogressanimation?file=app%2Fprogress-bar-determinate-example.html

…in mat-form-field-flex. This approach is more easily overriden by partner team that customizes the look and feel of Material components.

…it caused the outline box to be squished on projects that uses the mixin.

…uffer setting

crisbeto

What is the use case for this output? You should be able to do the same by attaching a transitionend event on the outside.

glouischandra · 2018-07-27T19:57:23Z

crisbeto - we need a notifier for when the animation completed to avoid it being cutoff if we're just relying on the value. This demo shows the problem: https://stackblitz.com/edit/matprogressanimation?file=app%2Fprogress-bar-determinate-example.html

If we attach transitionend on mat-progress-bar tag would it be hard to distinguish on which element animation ended? I haven't tried this but would all transitionend on value or bufferValue bubble up together? The event added here is specific for value transition and it feels cleaner that way. Thoughts?

crisbeto · 2018-07-27T20:02:11Z

You can look at the event.target to filter out only the events that you care about.

glouischandra · 2018-07-27T20:13:55Z

Thanks Cris, cmiiw: that works but I would need to know the specific details of elements inside mat-progress-bar which I think breaks encapsulation of this component.

jelbourn

@glouischandra @crisbeto Using just transitionend is definitely preferable from the library maintainer perspective, but I'm fine adding the extra output.

jelbourn · 2018-07-27T19:38:11Z

src/lib/progress-bar/progress-bar.spec.ts

 import {Location} from '@angular/common';
 import {MatProgressBarModule} from './index';
+import { MatProgressBar } from './progress-bar';


Omit spaces in braces

jelbourn · 2018-07-27T19:38:34Z

src/lib/progress-bar/progress-bar.ts

@@ -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;


This should be single quotes

jelbourn · 2018-07-27T19:42:30Z

src/lib/progress-bar/progress-bar.ts

+   * Also note, if multiple event update is thrown in quick succession the last event completion is 
+   * the one firing the output.
+   */
+  @Output() valueAnimationEnd = new EventEmitter<number>();


I would make the name here animationEnd

jelbourn · 2018-07-27T19:46:25Z

src/lib/progress-bar/progress-bar.ts

+   * Also note, if multiple event update is thrown in quick succession the last event completion is 
+   * the one firing the output.
+   */
+  @Output() valueAnimationEnd = new EventEmitter<number>();


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

Created event interface, looks more flexible now, thanks!

jelbourn · 2018-07-27T20:24:23Z

src/lib/progress-bar/progress-bar.ts

+  /** 
+   * 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.


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.

jelbourn · 2018-07-27T20:25:13Z

src/lib/progress-bar/progress-bar.ts

+
+  /** Attaches value bar element animation end listener. */
+  ngAfterViewInit() {
+    if(this.mode === 'determinate' || this.mode === 'buffer') {


Space after if (here and below)

jelbourn · 2018-07-27T20:26:08Z

src/lib/progress-bar/progress-bar.ts

@@ -113,6 +131,21 @@ export class MatProgressBar extends _MatProgressBarMixinBase implements CanColor
      return {transform: `scaleX(${scale})`};
    }
  }
+
+  /** Attaches value bar element animation end listener. */


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

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

jelbourn · 2018-07-27T20:26:51Z

src/lib/progress-bar/progress-bar.ts

+    }
+  }
+
+  /** Removes value bar element animation end listener. */


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

jelbourn · 2018-07-27T20:28:00Z

src/lib/progress-bar/progress-bar.spec.ts

@@ -118,6 +120,53 @@ describe('MatProgressBar', () => {
      expect(progressElement.componentInstance.mode).toBe('buffer');
    });
  });
+
+  describe('animation trigger on determinate setting', () => {


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

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

Check that event is not bound?

On animation === noop, always call output on value change?

Or would it be something else?

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.

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.

jelbourn · 2018-07-27T20:33:30Z

src/lib/progress-bar/progress-bar.ts

+
+  /** Attaches value bar element animation end listener. */
+  ngAfterViewInit() {
+    if(this.mode === 'determinate' || this.mode === 'buffer') {


This should also check the _animationMode

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

josephperrott · 2018-07-31T15:29:05Z

src/lib/progress-bar/progress-bar.ts

+
+  /** Attaches value bar element animation end listener. */
+  ngAfterViewInit() {
+    if(this.mode === 'determinate' || this.mode === 'buffer') {


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

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?

crisbeto · 2018-07-31T18:48:53Z

src/lib/progress-bar/progress-bar.ts

+  /** Attaches value bar element animation end listener. */
+  ngAfterViewInit() {
+    if(this.mode === 'determinate' || this.mode === 'buffer') {
+      this._valueAnimationSubscription = fromEvent(this.primaryValueBar.nativeElement, 'transitionend')


This needs a couple of things:

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.

The event should be bound outside the NgZone and it should re-enter the zone only when it emits.

Couple questions on these before I make the change:

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?

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.

That's true, but it's more of a future-proofing thing in case we decide to put something into the element.

It's only really necessary if we end up doing point 1, otherwise it would be redundant.

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

crisbeto · 2018-07-31T18:49:39Z

src/lib/progress-bar/progress-bar.ts

+  @Output() valueAnimationEnd = new EventEmitter<number>();
+
+  /** Reference to animation end subscription to be unsubscribed on destroy. */
+  private _valueAnimationSubscription: Subscription | null;


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

Nice tip! makes code more clean.

jelbourn

Mostly looks look, just a last few comments

jelbourn · 2018-08-01T19:56:35Z

src/lib/progress-bar/progress-bar.ts

+  /** Callback function on animation end, emit animationEnd event on determinate and buffer mode. */
+  private emitAnimationEnd(): void {
+    if (this.mode === 'determinate' || this.mode === 'buffer') {
+      this.animationEnd.next({ value: this.value});


Omit spaces in braces

jelbourn · 2018-08-01T19:59:44Z

src/lib/progress-bar/progress-bar.ts

+  /**
+   * Event that indicates animation end on value bar. This event will not be emitted for
+   * animations that doesn't end, i.e. indeterminate and query. Also if animation is
+   * disabled, this event will not emit.


Slight rewording:

Event emitted when animation of the primary progress bar completes. This event will not be emitted when animations are disabled, nor will it be emitted for modes with continuous animations (indeterminate and query).

jelbourn · 2018-08-01T20:00:47Z

src/lib/progress-bar/progress-bar.ts

@@ -82,6 +94,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;


This property should be prefixed with an underscore to indicate that it is not part of the public API surface

…ations mode on ProgressBar

jelbourn

LGTM aside from one last comment

jelbourn · 2018-08-02T16:58:47Z

src/lib/progress-bar/progress-bar.ts

@@ -27,6 +35,11 @@ export class MatProgressBarBase {
  constructor(public _elementRef: ElementRef) { }
 }

+/** Last animation end data. */
+export interface AnimationEndData {


Ah, one last thing I missed earlier- this should be ProgressAnimationEnd; having Data at the end is inconsistent with other events around the library

josephperrott · 2018-08-03T18:18:16Z

src/lib/progress-bar/progress-bar.ts

+    this._animationEndSubscription.unsubscribe();
+  }
+
+  /** Callback function on animation end, emit animationEnd event on determinate and buffer mode. */


Method comment should not describe when its used, only what it does. Should instead be:

/** Emit an animationEnd event if in determinate or buffer mode. */

ngbot · 2018-08-22T14:32:19Z

Hi @glouischandra! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-08-27T16:59:25Z

Hi @glouischandra! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-08-27T16:59:32Z

Hi @glouischandra! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-08-27T16:59:34Z

Hi @glouischandra! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-08-27T16:59:44Z

Hi @glouischandra! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-08-27T17:00:16Z

Hi @glouischandra! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

angular-automatic-lock-bot · 2019-09-09T12:03:37Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

glouischandra added 5 commits June 13, 2018 13:10

Modified Material FormField template so outline DOM is contained with…

4e9b321

…in mat-form-field-flex. This approach is more easily overriden by partner team that customizes the look and feel of Material components.

Removed extra whitespace in previous commit

92c9bd1

Removed bottom text styling from form-field-outline-theme.scss since …

c22ba7b

…it caused the outline box to be squished on projects that uses the mixin.

Merge remote-tracking branch 'upstream/master'

2bff7be

Add CSS transition event end output to progress bar determinate and b…

a759e1b

…uffer setting

glouischandra requested review from crisbeto, jelbourn and josephperrott as code owners July 27, 2018 19:06

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 27, 2018

Merge remote-tracking branch 'upstream/master'

0496f49

crisbeto reviewed Jul 27, 2018

View reviewed changes

jelbourn added the G This is is related to a Google internal issue label Jul 27, 2018

jelbourn reviewed Jul 27, 2018

View reviewed changes

josephperrott reviewed Jul 31, 2018

View reviewed changes

crisbeto reviewed Jul 31, 2018

View reviewed changes

Addressing feedback from Material Team

da7c38c

jelbourn added the target: major This PR is targeted for the next major release label Aug 1, 2018

jelbourn reviewed Aug 1, 2018

View reviewed changes

Addressing follow up comment. Also added more test cases and NoopAnim…

35f5f9a

…ations mode on ProgressBar

jelbourn approved these changes Aug 2, 2018

View reviewed changes

jelbourn added the pr: lgtm label Aug 2, 2018

Rename AnimationEndData to ProgressAnimationEnd

e7150a3

jelbourn added the action: merge The PR is ready for merge by the caretaker label Aug 2, 2018

josephperrott approved these changes Aug 3, 2018

View reviewed changes

Update comment per Joey's comment

83419bb

Merge remote-tracking branch 'upstream/master'

e4e002b

glouischandra added 2 commits August 23, 2018 17:04

Merge remote-tracking branch 'upstream/master'

a554065

Merge branch 'master' into master

3e8fd82

jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Aug 24, 2018

glouischandra added 2 commits August 23, 2018 17:39

Run lint after merge

fac6c90

Merge branch 'master' of https://github.com/glouischandra/material2

e4637a9

Merge remote-tracking branch 'upstream/master'

c5a9bc0

jelbourn merged commit 6a1a707 into angular:master Aug 28, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019

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

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

Conversation

glouischandra commented Jul 27, 2018

crisbeto left a comment

Choose a reason for hiding this comment

glouischandra commented Jul 27, 2018

crisbeto commented Jul 27, 2018

glouischandra commented Jul 27, 2018

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngbot bot commented Aug 22, 2018

ngbot bot commented Aug 27, 2018

ngbot bot commented Aug 27, 2018

ngbot bot commented Aug 27, 2018

ngbot bot commented Aug 27, 2018

ngbot bot commented Aug 27, 2018

angular-automatic-lock-bot bot commented Sep 9, 2019