feat(progress-spinner): add support for custom stroke-width #4113

devversion · 2017-04-15T17:45:05Z

Adds support for custom stroke-width of the progress-spinner elements.

josephperrott · 2017-04-15T21:07:18Z

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

@@ -33,8 +33,8 @@ $mat-progress-spinner-viewport-size: 100px !default;
  path {
    fill: transparent;

-    // Stroke width of 10px defines stroke as 10% of the viewBox.
-    stroke-width: $mat-progress-spinner-stroke-width;
+    // Stroke width is by default 10px, which is relatively to the viewBox size 10%.


It currently is centered properly in the viewBox in the getSvgArc method, we define pathRadius as 40 and radius as 50. If we allow the strokeWidth to be set we will need to make a corresponding adjustment to the pathRadius.

Thought we need to keep in mind getSvgArc is called ~60 times/sec, so ideally we don't want to recalculate the pathRadius value every time if we can avoid it, though it is a pretty trivial calculation compared to determining easing values.

That's a good point. In theory we could cache the pathRadius when the stroke width is updated through a setter. But I feel like it's not worth passing a cached value to the getSvgArc function (due to the trivial subtraction).

Let me know what you think. Happy to implement the caching if desired.

josephperrott · 2017-04-15T21:15:16Z

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

@@ -103,6 +107,11 @@ export class MdProgressSpinner implements OnDestroy {
    this._cleanupIndeterminateAnimation();
  }

+  /** Stroke width of the progress spinner. By default uses 10px as stroke width. */
+  @Input()


why not just use

@Input() strokeWidth: number = DEFAULT_STROKE_WIDTH;

Then it will have the default value, otherwise will use the input bound value.

I wanted to make a getter/setter because I wanted to avoid that the developer sets the value to 0 or falsy values.

This will still break if the consumer sets it to something like "abc". IMO the getter/setter here is overkill.

Agreed. We can just set the default stroke-width as initial value.

josephperrott · 2017-04-15T21:16:06Z

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

@@ -5,5 +5,5 @@
 -->
 <svg viewBox="0 0 100 100"
     preserveAspectRatio="xMidYMid meet">
-  <path></path>
+  <path [style.strokeWidth]="strokeWidth"></path>


If the value is being set in the style attribute here, should it be removed from the scss file?

I think I removed it?

Yep, I just missed it sorry.

crisbeto

Needs a little bit of cleanup.

crisbeto · 2017-04-16T10:49:33Z

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

@@ -108,6 +108,29 @@ describe('MdProgressSpinner', () => {
    expect(progressElement.componentInstance.interdeterminateInterval).toBeFalsy();
  });

+  it('should allow a custom stoke width', () => {


stoke->stroke

crisbeto · 2017-04-16T10:50:14Z

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

+
+    fixture.detectChanges();
+
+    expect(parseInt(pathElement.style.strokeWidth))


There's no need to parse the string to an int here. You can just expect it to be '10'.

There is a need for that. Safari 9 adds the px suffix to the strokeWidth value. We could have also done it through toContain but parseInt and toBe is more safe here.

crisbeto · 2017-04-16T10:51:40Z

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

@@ -25,6 +25,9 @@ const endIndeterminate = 80;
 /* Maximum angle for the arc. The angle can't be exactly 360, because the arc becomes hidden. */
 const MAX_ANGLE = 359.99 / 100;

+// Stroke width is by default 10px, which is relatively to the viewBox size 10%.


Not sure whether this comment is very helpful. We'd also need to keep it updated if we change the default value. Something like "Default stroke width as a percentage of the viewBox" might be better.

crisbeto · 2017-04-16T10:53:14Z

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

@@ -103,6 +107,11 @@ export class MdProgressSpinner implements OnDestroy {
    this._cleanupIndeterminateAnimation();
  }

+  /** Stroke width of the progress spinner. By default uses 10px as stroke width. */
+  @Input()


This will still break if the consumer sets it to something like "abc". IMO the getter/setter here is overkill.

crisbeto · 2017-04-16T10:57:43Z

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

+    fixture.detectChanges();
+
+    expect(parseInt(pathElement.style.strokeWidth))
+      .toBe(10, 'Expected stroke width to fallback to 10px.');


I'm not sure whether we should be testing the exact default value here since it'll start failing if we change it. I suggest either importing the DEFAULT_STROKE_WIDTH and checking against it, or just checking that it has a valid stroke width in the first place.

I think importing might be the best solution here.

crisbeto

LGTM, one minor nit.

crisbeto · 2017-04-17T10:25:17Z

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

@@ -73,6 +75,9 @@ export class MdProgressSpinner implements OnDestroy {
  private _value: number;
  private _color: string = 'primary';

+  /** Stroke width of the progress spinner. By default uses 10px as stroke width. */


I don't think that this should have the By default uses 10px as stroke width. since the default may change and I doubt that we'd remember to update the comment.

wiwski · 2017-04-20T13:59:25Z

Hi there,
Any news about this merge ?

* Adds support for custom `stroke-width` of the progress-spinner elements. Fixes angular#3934

angular-automatic-lock-bot · 2019-09-07T15:01:36Z

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

devversion added the pr: needs review label Apr 15, 2017

devversion assigned crisbeto Apr 15, 2017

devversion requested a review from crisbeto April 15, 2017 17:45

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

josephperrott reviewed Apr 15, 2017

View reviewed changes

crisbeto reviewed Apr 16, 2017

View reviewed changes

devversion force-pushed the feat/progress-spinner-stroke branch 2 times, most recently from af36ecb to c9a9e06 Compare April 16, 2017 12:43

crisbeto approved these changes Apr 17, 2017

View reviewed changes

crisbeto added pr: lgtm and removed pr: needs review labels Apr 17, 2017

devversion added the action: merge The PR is ready for merge by the caretaker label Apr 17, 2017

feat(progress-spinner): add support for custom stroke-width

608babe

* Adds support for custom `stroke-width` of the progress-spinner elements. Fixes angular#3934

devversion force-pushed the feat/progress-spinner-stroke branch from ac419c5 to 608babe Compare April 20, 2017 14:48

kara merged commit b846a27 into angular:master Apr 21, 2017

devversion deleted the feat/progress-spinner-stroke branch November 11, 2017 10:20

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): add support for custom stroke-width #4113

feat(progress-spinner): add support for custom stroke-width #4113

devversion commented Apr 15, 2017

josephperrott Apr 15, 2017

devversion Apr 16, 2017

josephperrott Apr 15, 2017

devversion Apr 15, 2017

crisbeto Apr 16, 2017

devversion Apr 16, 2017

josephperrott Apr 15, 2017

devversion Apr 15, 2017

josephperrott Apr 15, 2017

crisbeto left a comment

crisbeto Apr 16, 2017

crisbeto Apr 16, 2017

devversion Apr 16, 2017

crisbeto Apr 16, 2017

crisbeto Apr 16, 2017

crisbeto Apr 16, 2017

devversion Apr 16, 2017

crisbeto left a comment

crisbeto Apr 17, 2017

wiwski commented Apr 20, 2017

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


		fixture.detectChanges();

		expect(parseInt(pathElement.style.strokeWidth))

feat(progress-spinner): add support for custom stroke-width #4113

feat(progress-spinner): add support for custom stroke-width #4113

Conversation

devversion commented Apr 15, 2017

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

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

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

wiwski commented Apr 20, 2017

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