fix(progress): progress buffer defaults to 0 #5352
Conversation
datvm
changed the title
|
@asyncLiz hi, anything else for this PR? |
progress/internal/linear-progress.ts
Outdated
| @@ -30,14 +31,15 @@ export class LinearProgress extends Progress { | |||
| }; | |||
| const dotStyles = { | |||
| transform: `scaleX(${ | |||
| (this.indeterminate ? 1 : this.buffer / this.max) * 100 | |||
| // == null is used to check for both null and undefined when buffer attribute is removed | |||
| (this.indeterminate || this.buffer == null || this.buffer < 0 ? 1 : this.buffer / this.max) * 100 | |||
There was a problem hiding this comment.
This is starting to get hard to grok, let's split it out and simplify it.
// use ?? for null and undefined when the buffer attribute is removed.
const bufferValue = this.buffer ?? 0;
const hasBuffer buffer > 0;
const dotSize = this.indeterminate || !hasBuffer ? 1 : bufferValue / this.max;
const dotStyles = {
transform: `scaleX(${dotSize * 100}%)`
};
progress/internal/linear-progress.ts
Outdated
| }%)`, | ||
| }; | ||
|
|
||
| // Only display dots when visible - this prevents invisible infinite | ||
| // animation. | ||
| const hideDots = | ||
| this.indeterminate || this.buffer >= this.max || this.value >= this.max; | ||
| this.indeterminate || this.buffer == null || this.buffer < 0 || this.buffer >= this.max || this.value >= this.max; |
There was a problem hiding this comment.
And use the new variables here
const hideDots =
this.indeterminate || !hasBuffer || bufferValue >= this.max || this.value >= this.max;
progress/internal/linear-progress.ts
Outdated
| @@ -16,8 +16,9 @@ import {Progress} from './progress.js'; | |||
| export class LinearProgress extends Progress { | |||
| /** | |||
| * Buffer amount to display, a fraction between 0 and `max`. | |||
| * If negative, the buffer is not displayed. | |||
There was a problem hiding this comment.
Hm, actually what about using 0 as the default value to indicate that there is no buffer? I think that makes more better sense than using negative to indicate it.
If there's 0 buffer after all, there is no buffer.
There was a problem hiding this comment.
@asyncLiz thanks, the latest commit should address the above suggestions
progress/internal/linear-progress.ts
Outdated
| @@ -28,16 +29,20 @@ export class LinearProgress extends Progress { | |||
| (this.indeterminate ? 1 : this.value / this.max) * 100 | |||
| }%)`, | |||
| }; | |||
|
|
|||
| const bufferValue = Math.min(this.buffer ?? 0, this.max); | |||
There was a problem hiding this comment.
I'm not sure I entirely follow why we need Math.min here to change bufferValue to max. Is it to avoid bufferValue >= this.max for const hideDots?
There was a problem hiding this comment.
Yes, so we do not need to worry about that logic with dotStyles anymore though I suppose the hideDots would take care of it anyway right?
There was a problem hiding this comment.
Yea I believe so. Do you mind not clamping it and keeping the bufferValue >= this.max check?
Semantically "bufferValue" as changed doesn't reflect this.buffer, it's really bufferValueOrMax. It's a bit confusing to read, and may inadvertently cause problems if future changes don't realize that nuance.
There was a problem hiding this comment.
@asyncLiz sure I did in the last commit.
progress/internal/linear-progress.ts
Outdated
| }; | ||
|
|
||
| // Only display dots when visible - this prevents invisible infinite | ||
| // animation. | ||
| const hideDots = | ||
| this.indeterminate || this.buffer >= this.max || this.value >= this.max; | ||
| this.indeterminate || !hasBuffer || this.value >= this.max; |
There was a problem hiding this comment.
Last change I think! We still need the max check, !hasBuffer || bufferValue >= this.max
There was a problem hiding this comment.
Oops 😬 sorry I forgot about that one after removing the previous one. It should be updated now.
asyncLiz
changed the title
copybara-service
bot
merged commit 2613de4
into
material-components:main
Fix #5251 .