Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix(progress-spinner): coerce string values #7791

Merged
merged 1 commit into from
Nov 10, 2017
Merged

fix(progress-spinner): coerce string values #7791

merged 1 commit into from
Nov 10, 2017

Conversation

crisbeto
Copy link
Member

Coerces any string values that are passed to value, diameter and strokeWidth to numbers.

Fixes #7790.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 14, 2017
josephperrott
josephperrott approved these changes Oct 16, 2017
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, a couple idea/questions.

src/lib/progress-spinner/progress-spinner.ts Outdated
@@ -86,6 +87,7 @@ export class MatProgressSpinner extends _MatProgressSpinnerMixinBase implements
private _value: number;
private readonly _baseSize = 100;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a module const? Since it is only read during construction, and can't be modified?

src/lib/progress-spinner/progress-spinner.ts
@@ -121,7 +132,7 @@ export class MatProgressSpinner extends _MatProgressSpinnerMixinBase implements
}
set value(newValue: number) {
if (newValue != null && this.mode === 'determinate') {
this._value = Math.max(0, Math.min(100, newValue));
this._value = Math.max(0, Math.min(100, coerceNumberProperty(newValue)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We created a clamp method in MdProgressBar for this, should we do similar here? Potentially moving clamp to a util?

If so it might be in a follow up PR, but thought I would throw it out there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to have a clamp function because it was used in a couple of places, but now it's only used once so it should be fine having it inline.

@josephperrott
Copy link
Member

@crisbeto Add pr: merge ready once everything is in order.

@crisbeto crisbeto force-pushed the 7790/spinner-coercion branch from 6317583 to 5defc78 Compare October 16, 2017 16:52
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Oct 16, 2017
@crisbeto crisbeto force-pushed the 7790/spinner-coercion branch from 5defc78 to 24a8062 Compare October 16, 2017 16:56
@andrewseguin
Copy link
Contributor

Needs rebase.

@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Nov 2, 2017
@crisbeto crisbeto force-pushed the 7790/spinner-coercion branch from 24a8062 to 41c5a2d Compare November 2, 2017 17:47
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Nov 2, 2017
@crisbeto
Copy link
Member Author

crisbeto commented Nov 2, 2017

Rebased.

@josephperrott
Copy link
Member

Needs rebase

@crisbeto
fix(progress-spinner): coerce string values
09b3afe 
Coerces any string values that are passed to `value`, `diameter` and `strokeWidth` to numbers.

Fixes angular#7790.
@crisbeto crisbeto force-pushed the 7790/spinner-coercion branch from 41c5a2d to 09b3afe Compare November 8, 2017 12:38
@crisbeto crisbeto requested a review from kara as a code owner November 8, 2017 12:38
@crisbeto
Copy link
Member Author

crisbeto commented Nov 8, 2017

Rebased @josephperrott.

@crisbeto crisbeto mentioned this pull request Nov 10, 2017
Closed
@willshowell willshowell mentioned this pull request Nov 10, 2017
Closed
@josephperrott josephperrott merged commit b6712f8 into angular:master Nov 10, 2017
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@josephperrott josephperrott josephperrott approved these changes

@jelbourn jelbourn Awaiting requested review from jelbourn

@kara kara Awaiting requested review from kara

Assignees

@josephperrott josephperrott

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Progress Spinner diameter and strokeWidth bound vs unbound
4 participants
@crisbeto @josephperrott @andrewseguin @googlebot