Skip to content

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

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
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

@crisbeto crisbeto commented Oct 14, 2017

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
OnChanges {

private _value: number;
private readonly _baseSize = 100;
Copy link
Member

@josephperrott josephperrott Oct 16, 2017

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

@josephperrott josephperrott Oct 16, 2017

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

@crisbeto crisbeto Oct 16, 2017

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

josephperrott commented Oct 16, 2017

@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

andrewseguin commented Nov 2, 2017

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

josephperrott commented Nov 7, 2017

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

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

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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