feat(progress-spinner): switch to css-based animation #6551

crisbeto · 2017-08-19T18:02:57Z

Reworks the progress spinner to use a CSS-based indeterminate animation. For IE and Edge where we can't pull of the animation reliably, it falls back to a non-spec animation.

Demo: https://material-cb7ec.firebaseapp.com/progress-spinner

jelbourn

LGTM, one minor comment

jelbourn · 2017-08-21T18:12:04Z

src/lib/progress-spinner/progress-spinner.scss

+    // stylelint-enable
+  }
+
+  @keyframes mat-progress-spinner-stroke-rotate-fallback {


Add comment describing this animation?

// For IE11 and Edge, we fall back to simply rotating the spinner because // animating stroke-dashoffset is not supported. The fallback uses multiple // iterations to vary where the spin "lands".

Done. FWIW, animating stroke-dashoffset is supported in Edge, but rotation the circle with a CSS transform isn't.

kara · 2017-08-23T18:05:16Z

@crisbeto @jelbourn This PR breaks internal tests because some projects are relying on being able to set [style.width.px] and [style.height.px] on the spinner element. Thoughts?

crisbeto · 2017-08-23T18:37:12Z

@jelbourn The reason I removed the width and height from the CSS was because we need control over the height due to the strokeWidth option. If the stroke width is greater than the default 10px it will overflow the element, which is why I ended up making it smaller to compensate. My guess is that the option isn't widely used anyway so we could get rid of it and let users override it with CSS. If they decide to override it, they can also tweak the margins/padding/width or height depending on their setup.

jelbourn · 2017-09-22T20:35:59Z

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

-    return this.mode == 'determinate' ? 0 : null;
-  }
+  /** Tracks diameters of existing instances to de-dupe generated styles. */
+  static diameters = new Set<number>([100]);


Comment about 100 being the default diameter?

jelbourn · 2017-09-22T20:36:35Z

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

-    if (this._interdeterminateInterval) {
-      clearInterval(this._interdeterminateInterval);
+  set diameter(size: number) {
+    this._setDiameterClass(size);


Break the content of this setter into its own function?

jelbourn · 2017-09-22T20:38:50Z

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

    }

    return null;
  }
+
+  private _setDiameterClass(size: number): void {


JsDoc descriptions for these functions?

crisbeto · 2017-09-22T20:52:48Z

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

    }
  }

-  _getStrokeDashOffset() {
+  get _circleRadius() {
+    return this._diameter / 2 - 5;


Where does the 5 come from? I assume it's half the stroke width?

I believe that's what it's supposed to be, yes; fixing.

crisbeto · 2017-09-22T20:55:04Z

src/lib/progress-spinner/progress-spinner.scss

+  $end: (1 - 0.8) * $_mat-progress-spinner-default-circumference; // end the animation at 80%
+  $fallback-iterations: 4;
+
+  @keyframes mat-progress-spinner-stroke-rotate-100 {


Should we keep this one around, considering that the keyframes are being generated at runtime?

We won't generate a style tag at runtime for the default diameter - only for custom diameters (the set starts with 100 included).

crisbeto · 2017-09-22T20:56:54Z

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

 // Boilerplate for applying mixins to MdProgressSpinner.
 /** @docs-private */
 export class MdProgressSpinnerBase {
  constructor(public _renderer: Renderer2, public _elementRef: ElementRef) {}
 }
 export const _MdProgressSpinnerMixinBase = mixinColor(MdProgressSpinnerBase, 'primary');

+/* tslint:disable:max-line-length */
+const INDETERMINATE_ANIMATION_TEMPLATE = `
+  .mat-progress-spinner-DIAMETER.mat-progress-spinner-indeterminate-animation[mode="indeterminate"] circle {


Rather than constructing this selector, it might be better to just assign the animation-name: mat-progress-spinner-stroke-rotate-DIAMETER as an inline style. That way people can still override the timing and duration through CSS.

Oh hey, that's a great idea. Should simplify this a lot.

crisbeto · 2017-09-22T20:58:55Z

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

+  set diameter(size: number) {
+    this._setDiameterClass(size);
+    this._diameter = size;
+    if (!MdProgressSpinner.diameters.has(this.diameter)) {


Since we don't support the spec animation on IE and Edge, we shouldn't append the keyframes either.

crisbeto

LGTM, one nit. I'm not allowed to approve my own PR.

crisbeto · 2017-09-23T08:05:46Z

src/lib/progress-spinner/progress-spinner.html

+    [style.animation-name]="'mat-progress-spinner-stroke-rotate-' + diameter"
+    [style.stroke-dashoffset.px]="_strokeDashOffset"
+    [style.stroke-dasharray.px]="_strokeCircumference"
+    [style.transform.rotate]="'360deg'"


Why do we need this? It doesn't look like it does anything.

You know, I thought that was from your commit, but now I have no idea. Maybe it's a weird rebase error? Don't remember adding it.

Hmm, I haven't pulled in your changes into my local branch yet. I tried checking it out and it's not in there.

I must have been testing something at some point? I'll remove it, in any case.

crisbeto · 2017-09-29T15:37:07Z

@kara this one needs a rebase.

Reworks the progress spinner to use a CSS-based indeterminate animation. For IE and Edge where we can't pull of the animation reliably, it falls back to a non-spec animation.

pabx06 · 2017-10-23T03:11:00Z

@kara i am using the latest beta.
Changing the width & height style i still get 100px*100px spinner ...
i am using the spinner as

<mat-form-field ...> <input...> <span matSuffix style="height: 18px" align="end" *ngIf="searching"> <mat-progress-spinner mode="indeterminate" ng-disabled="true" style="height: 18px" align="end"> </mat-progress-spinner> </mat-form-field>

on debounced by 300ms keystroke

any idea how i could set the spinner to fit the text input field ?

angular-automatic-lock-bot · 2019-09-07T11:02:10Z

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

crisbeto added the pr: needs review label Aug 19, 2017

crisbeto assigned jelbourn Aug 19, 2017

crisbeto requested review from jelbourn and josephperrott as code owners August 19, 2017 18:02

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 19, 2017

crisbeto force-pushed the progress-spinner-rewrite branch 2 times, most recently from 283306a to c5a54d0 Compare August 20, 2017 16:59

jelbourn approved these changes Aug 21, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Aug 21, 2017

crisbeto force-pushed the progress-spinner-rewrite branch from c5a54d0 to b759be0 Compare August 21, 2017 18:26

crisbeto added the action: merge The PR is ready for merge by the caretaker label Aug 21, 2017

kara added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Aug 23, 2017

crisbeto force-pushed the progress-spinner-rewrite branch from b759be0 to a673da2 Compare August 23, 2017 06:40

kara added needs: discussion Further discussion with the team is needed before proceeding and removed action: merge The PR is ready for merge by the caretaker labels Aug 23, 2017

josephperrott mentioned this pull request Aug 30, 2017

memory leak Progress spinner #5676

Closed

jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 15, 2017

crisbeto force-pushed the progress-spinner-rewrite branch 6 times, most recently from 1d9e9d8 to f7f4b07 Compare September 22, 2017 19:45

kara added pr: needs review and removed pr: lgtm needs: discussion Further discussion with the team is needed before proceeding labels Sep 22, 2017

jelbourn reviewed Sep 22, 2017

View reviewed changes

kara mentioned this pull request Sep 22, 2017

feat(progress-spinner): switch to css-based animation #7265

Closed

crisbeto commented Sep 22, 2017

View reviewed changes

kara force-pushed the progress-spinner-rewrite branch 2 times, most recently from 9b453c3 to 71c5cb6 Compare September 22, 2017 22:19

crisbeto commented Sep 23, 2017

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Sep 26, 2017

crisbeto mentioned this pull request Sep 26, 2017

md-spinner memory leaks #7325

Closed

crisbeto added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 29, 2017

crisbeto and others added 2 commits October 4, 2017 19:00

feat(progress-spinner): switch to css-based animation

282c87f

Reworks the progress spinner to use a CSS-based indeterminate animation. For IE and Edge where we can't pull of the animation reliably, it falls back to a non-spec animation.

feat(progress-spinner): add support for custom diameters

0504e41

kara force-pushed the progress-spinner-rewrite branch from 71c5cb6 to 0504e41 Compare October 5, 2017 02:16

kara added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Oct 5, 2017

kara merged commit 630dfad into angular:master Oct 5, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(progress-spinner): switch to css-based animation #6551

feat(progress-spinner): switch to css-based animation #6551

crisbeto commented Aug 19, 2017 •

edited

Loading

jelbourn left a comment

jelbourn Aug 21, 2017

crisbeto Aug 21, 2017

kara commented Aug 23, 2017

crisbeto commented Aug 23, 2017 •

edited

Loading

jelbourn Sep 22, 2017

jelbourn Sep 22, 2017

jelbourn Sep 22, 2017

crisbeto Sep 22, 2017 •

edited

Loading

kara Sep 22, 2017

crisbeto Sep 22, 2017

kara Sep 22, 2017

crisbeto Sep 22, 2017

kara Sep 22, 2017

crisbeto Sep 22, 2017

kara Sep 22, 2017

crisbeto left a comment

crisbeto Sep 23, 2017

kara Sep 25, 2017

crisbeto Sep 25, 2017

kara Sep 25, 2017

crisbeto commented Sep 29, 2017

pabx06 commented Oct 23, 2017

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

feat(progress-spinner): switch to css-based animation #6551

feat(progress-spinner): switch to css-based animation #6551

Conversation

crisbeto commented Aug 19, 2017 • edited Loading

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

kara commented Aug 23, 2017

crisbeto commented Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Sep 22, 2017 • edited Loading

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

crisbeto 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

crisbeto commented Sep 29, 2017

pabx06 commented Oct 23, 2017

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

crisbeto commented Aug 19, 2017 •

edited

Loading

crisbeto commented Aug 23, 2017 •

edited

Loading

crisbeto Sep 22, 2017 •

edited

Loading