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.
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
fix(progressbar): display progressbar correctly for invalid max value #3400
fix(progressbar): display progressbar correctly for invalid max value #3400
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3400 +/- ##
=========================================
+ Coverage 91.1% 91.1% +<.01%
=========================================
Files 95 95
Lines 2787 2789 +2
Branches 519 520 +1
=========================================
+ Hits 2539 2541 +2
Misses 189 189
Partials 59 59
Continue to review full report at Codecov.
|
@chrstnbrn |
9c88187
to
a23b7b1
Compare
a23b7b1
to
a25b7ce
Compare
@peterblazejewicz I changed the code to handle negative values. I also updates the docs to say that max should be a positive number. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @chrstnbrn!
Thanks for updating the docs and adding tests.
However, I don't think the implemented behaviour is correct though → you should adjust the max
value, not the percentage value.
Bootstrap defaults to 100 is max
is not specified, so let's just adjust the max
when setting it.
Actually the standard <progress>
element does the same thing, but they default to 1
, not 100
(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress)
All your tests should return expect(progressCmp.getPercentValue()).toBe(25);
Having value=25 max=0
to show 100
is very confusing.
So, I'd suggest adjusting the max
value in the setter like so:
private _max: number;
@Input() set max(max: number) {
this._max = !isNumber(max) || max <= 0 ? 100 : max;
}
get max(): number {
return this._max;
}
And document the behaviour a little better (see comment in code).
- adjust max value when setting it
- make tests display
25
- update documentation
a25b7ce
to
7ccc031
Compare
Hi @maxokorokov, thank you for your feedback! I agree that adjusting the invalid max value makes more sense and made the changes you suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, @chrstnbrn, LGTM!
closes #3386
Before submitting a pull request, please make sure you have at least performed the following:
Currently the following is displayed when max is set to 0, null or undefined:
After this change the max value will be set to 100 if it is invalid so that a progressbar with a valid value can be displayed.