Skip to content
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

fix(progress): progress buffer defaults to 0 #5352

Merged
merged 6 commits into from
Feb 22, 2024
Merged

fix(progress): progress buffer defaults to 0 #5352

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8fa444c
fix(progress): progress buffer defaults to negative.
datvm Dec 27, 2023
e4b8ef5
Added null check for buffer (due to possibility of attribute removal)
datvm Dec 28, 2023
1e1048a
Merge branch 'main' into linear-progress-buffer
datvm Dec 28, 2023
a8a9d56
Buffer default value is now 0 and added some code cleanup.
datvm Feb 15, 2024
2512f55
Removed linear progress buffer clamping
datvm Feb 17, 2024
c7e26b5
Added back the buffer check vs. max value
datvm Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions progress/internal/linear-progress.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asyncLiz thanks, the latest commit should address the above suggestions

*/
@property({type: Number}) buffer = 1;
@property({type: Number}) buffer = -1;

// Note, the indeterminate animation is rendered with transform %'s
// Previously, this was optimized to use px calculated with the resizeObserver
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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}%)`
};

}%)`,
};

// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

And use the new variables here

const hideDots =
  this.indeterminate || !hasBuffer || bufferValue >= this.max || this.value >= this.max;

return html`
<div class="dots" ?hidden=${hideDots}></div>
<div class="inactive-track" style=${styleMap(dotStyles)}></div>
Expand Down