fix(progress-bar/progress-spinner): prevent animations when in a noopanimations module context #10881

josephperrott · 2018-04-16T18:29:16Z

mmalerba

change title to fix(progress-bar/progress-spinner): ...

mmalerba · 2018-04-16T18:45:35Z

src/lib/core/style/_noop-animation.scss

@@ -0,0 +1,9 @@
+// If the mat-animation-noop class is present on the components root element,
+// prevent non css animations from running.
+@mixin noop-animation() {


mat-noopable-animation?

I would also prefix it with an underscore

crisbeto · 2018-04-17T02:00:27Z

src/lib/core/style/_noop-animation.scss

+// If the mat-animation-noop class is present on the components root element,
+// prevent non css animations from running.
+@mixin noop-animation() {
+  @at-root .mat-animation-noop#{&} {


Shouldn't this one be wrapped by the at-root?

@at-root { .mat-animation-noop { ... } }

If you do

@at-root { .mat-animation-noop { ... } }

It ends up as
root-element-thing .mat-animation-noop, but we want root-element-thing.mat-animation-noop

crisbeto · 2018-04-17T02:03:42Z

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

  ViewEncapsulation
 } from '@angular/core';
+import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';


Wouldn't adding this import be considered a breaking change, in the case where somebody might not have the animations package? I think we had a discussion along the same lines with @mmalerba where we considered adding animations to a component, that didn't have any, a breaking change. That being said, most people should have platform-browser already.

I think this is different because it doesn't require a module to be loaded; for animations, the breaking change is that you need to include BrowserAnimationsModule at the root for it to work. For this, I don't think it actually breaks anything.

crisbeto · 2018-04-17T02:06:51Z

src/lib/progress-spinner/progress-spinner.ts

@@ -144,7 +146,8 @@ export class MatProgressSpinner extends _MatProgressSpinnerMixinBase implements

  constructor(public _elementRef: ElementRef,
              platform: Platform,
-              @Optional() @Inject(DOCUMENT) private _document: any) {
+              @Optional() @Inject(DOCUMENT) private _document: any,
+              @Optional() @Inject(ANIMATION_MODULE_TYPE) public _animationMode: string) {


The _animationMode param should also be marked as optional, in case anybody was extending the component.

jelbourn · 2018-04-17T19:22:55Z

src/demo-app/demo-app-module.ts

@@ -20,7 +20,7 @@ import {AccessibilityDemoModule} from './a11y/a11y-module';
 @NgModule({
  imports: [
    BrowserModule,
-    BrowserAnimationsModule,
+    NoopAnimationsModule,


We don't want to switch the demo-app completely to noop. It should only be for some specific section you want to test.

jelbourn · 2018-04-17T19:23:30Z

src/lib/core/style/_noop-animation.scss

@@ -0,0 +1,9 @@
+// If the mat-animation-noop class is present on the components root element,
+// prevent non css animations from running.
+@mixin noop-animation() {


I would also prefix it with an underscore

jelbourn · 2018-04-17T19:29:42Z

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

  ViewEncapsulation
 } from '@angular/core';
+import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';


I think this is different because it doesn't require a module to be loaded; for animations, the breaking change is that you need to include BrowserAnimationsModule at the root for it to work. For this, I don't think it actually breaks anything.

jelbourn · 2018-04-17T19:30:54Z

src/lib/progress-bar/progress-bar.scss


 $mat-progress-bar-height: 5px !default;
 $mat-progress-bar-full-animation-duration: 2000ms !default;
 $mat-progress-bar-piece-animation-duration: 250ms !default;


 .mat-progress-bar {
+  @include noop-animation();


I'm not a fan of the way the mixin is peppered throughout the file in so many places. Let's take a look next week to see what it would look like with the noop animations code in one place.

jelbourn · 2018-04-24T17:53:48Z

src/lib/core/style/_noop-animation.scss

+// If the mat-animation-noop class is present on the components root element,
+// prevent non css animations from running.
+@mixin _noop-animation() {
+  @at-root .mat-animation-noop#{&} {


Add comment that explains why @at-root is used here, perhaps with an example?

jelbourn · 2018-04-24T17:54:01Z

src/lib/core/style/_noop-animation.scss

+// prevent non css animations from running.
+@mixin _noop-animation() {
+  @at-root .mat-animation-noop#{&} {
+    transition-property: none;


You probably don't need transition-property

jelbourn · 2018-04-24T17:58:28Z

src/lib/core/style/_noop-animation.scss

+// prevent non css animations from running.
+// NOTE: Currently this mixin should only be used with components that do not
+// have any projected content.
+@mixin _noop-animation() {


_mat-noop-animation

can we call it noopable? it's not nooping the animation, it's just making it noopable in case you add the noop class

bennyboer · 2019-04-04T06:25:31Z

Hi there!

It does not make sense to me to disable animations on a progress spinner, as this is a functional animation not just for visuals.
Who would need progress spinners which do not move at all?

carlosperezortola · 2019-04-04T06:38:29Z

Bennyboer is correct. Why would you want to disable the progress bars? A loading screen looks then broken.

crisbeto · 2019-04-04T07:52:32Z

The main use case for this is something like e2e or screenshot tests where your results could be thrown off by the animation.

bennyboer · 2019-04-04T08:24:28Z

The main use case for this is something like e2e or screenshot tests where your results could be thrown off by the animation.

You can force the animations of the progress spinner using the MAT_PROGRESS_SPINNER_DEFAULT_OPTIONS injection token, but this is not documented anywhere.
I don't understand why there are no hidden switches then for these specific tests to turn off animations instead of turning them off by default when using the NoopAnimationsModule.

I created an issue as this is not the correct place to discuss that.

angular-automatic-lock-bot · 2019-09-10T17:02:27Z

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.}

josephperrott requested review from jelbourn, crisbeto and mmalerba April 16, 2018 18:29

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

mmalerba reviewed Apr 16, 2018

View reviewed changes

josephperrott force-pushed the animation-noop branch from 353d9ef to 4bedbef Compare April 16, 2018 18:52

crisbeto reviewed Apr 17, 2018

View reviewed changes

jelbourn reviewed Apr 17, 2018

View reviewed changes

josephperrott changed the title ~~fix(animations): prevent animations when in a noopanimations module context~~ fix(progress-bar/progress-spinner\): prevent animations when in a noopanimations module context Apr 24, 2018

josephperrott changed the title ~~fix(progress-bar/progress-spinner\): prevent animations when in a noopanimations module context~~ fix(progress-bar/progress-spinner): prevent animations when in a noopanimations module context Apr 24, 2018

josephperrott force-pushed the animation-noop branch 2 times, most recently from 2aa3839 to ee2ef0e Compare April 24, 2018 17:56

jelbourn reviewed Apr 24, 2018

View reviewed changes

josephperrott force-pushed the animation-noop branch from ee2ef0e to 780c2b5 Compare April 24, 2018 20:35

fix(animations): prevent animations when in a noopanimations module

eb6c4b8

josephperrott force-pushed the animation-noop branch from 780c2b5 to eb6c4b8 Compare April 27, 2018 23:00

josephperrott added the target: minor This PR is targeted for the next minor release label May 4, 2018

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels May 4, 2018

josephperrott merged commit 81b6a78 into angular:master May 4, 2018

josephperrott deleted the animation-noop branch May 4, 2018 20:12

bennyboer mentioned this pull request Apr 4, 2019

Progress bar/spinner animations with NoopAnimationsModule #15713

Closed

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

fix(progress-bar/progress-spinner): prevent animations when in a noopanimations module context #10881

fix(progress-bar/progress-spinner): prevent animations when in a noopanimations module context #10881

Uh oh!

Conversation

josephperrott commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bennyboer commented Apr 4, 2019

Uh oh!

carlosperezortola commented Apr 4, 2019

Uh oh!

crisbeto commented Apr 4, 2019

Uh oh!

bennyboer commented Apr 4, 2019

Uh oh!

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

Uh oh!

Uh oh!

josephperrott commented Apr 16, 2018 •

edited

Loading