feat(core): progress gauge#154
Open
coryrylan wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds the ChangesProgress Gauge feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
projects/core/src/bundle.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. projects/core/src/index.test.lighthouse.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. projects/core/src/progress-gauge/define.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
github-actions
Bot
added
scope(core)
scope(ci)
scope(docs)
dependencies
Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-progress-guage
branch
from
5675997 to
e315a0f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/vale/styles/config/vocabularies/Elements/accept.txt`:
- Around line 361-362: The Elements vocabulary entry currently includes generic
terms that should not be part of the shared docs vocabulary. Remove indicate and
utilization from the accept list in the Elements vocabulary file, and keep this
vocabulary limited to technical terms, component names, or abbreviations that
are specific to the project.
In `@projects/core/src/progress-gauge/progress-gauge.css`:
- Around line 4-21: The :host rule in progress-gauge.css fails
declaration-empty-line-before because there is no blank line before display;
update the :host block to add the required empty line separating the custom
property declarations from the standard declarations, keeping the fix within the
progress-gauge styles.
In `@projects/core/src/progress-gauge/progress-gauge.examples.ts`:
- Around line 114-123: The Dynamic example script creates an unmanaged interval
and uses a document-wide query, which can stack timers and keep mutating a
detached or wrong gauge on rerender. Update the inline module in
progress-gauge.examples.ts to scope lookups from the example’s own root
container instead of document, and keep a handle to the timer created in the
dynamic gauge example. Add teardown/cleanup for that setInterval when the
example is re-rendered or disposed so only one timer runs per instance.
In `@projects/core/src/progress-gauge/progress-gauge.test.axe.ts`:
- Around line 15-24: The axe test setup only waits for the first
nve-progress-gauge to stabilize, so the remaining gauges may still be rendering
when runAxe executes. Update the beforeEach in progress-gauge.test.axe.ts to
wait for every rendered ProgressGauge instance from the fixture, not just the
element returned by querySelector, so the DOM is fully settled before
accessibility checks run.
In `@projects/core/src/progress-gauge/progress-gauge.test.visual.ts`:
- Around line 19-24: The visual test template is always setting the nve-theme
attribute, even for the default baseline when theme is empty. Update template to
only call document.documentElement.setAttribute in the dark-theme case, and
leave the default document state untouched when theme === ''. Use the template
helper in progress-gauge.test.visual.ts as the place to guard the attribute
assignment.
In `@projects/core/src/progress-gauge/progress-gauge.ts`:
- Around line 22-26: The public version metadata for ProgressGauge still uses a
placeholder release value. Update the `@since` tag in the component docblock and
the ProgressGauge.metadata.version field so they reflect the real published
version instead of 0.0.0, and make the same change anywhere else in this diff
that duplicates the placeholder metadata.
- Around line 49-53: Normalize the progress gauge inputs before they are used in
geometry or ARIA: in progress-gauge.ts, clamp and sanitize `value` and `max` so
`NaN`, negative values, `max <= 0`, and `value > max` cannot flow into
rendering. Update `render()` and `updated()` to reuse the same normalized
numbers when computing the dash geometry and setting
`aria-valuenow`/`aria-valuemax`, using the `value` and `max` properties as the
source symbols to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a3a78298-8f17-4601-b576-e410d040447d
⛔ Files ignored due to path filters (2)
projects/core/.visual/progress-gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/progress-gauge.pngis excluded by!**/*.png
📒 Files selected for processing (16)
config/vale/styles/config/vocabularies/Elements/accept.txtprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/progress-gauge/define.tsprojects/core/src/progress-gauge/index.tsprojects/core/src/progress-gauge/progress-gauge.cssprojects/core/src/progress-gauge/progress-gauge.examples.tsprojects/core/src/progress-gauge/progress-gauge.test.axe.tsprojects/core/src/progress-gauge/progress-gauge.test.lighthouse.tsprojects/core/src/progress-gauge/progress-gauge.test.ssr.tsprojects/core/src/progress-gauge/progress-gauge.test.tsprojects/core/src/progress-gauge/progress-gauge.test.visual.tsprojects/core/src/progress-gauge/progress-gauge.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/progress-gauge.md
Comment on lines
+361
to
+362
| indicate | ||
| utilization |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Keep the Elements vocabulary scoped to docs-specific terms.
indicate and utilization are generic words, so adding them here broadens the shared docs vocabulary without a clear technical need.
As per coding guidelines, add new technical terms, component names, or abbreviations to the Elements vocabulary.
♻️ Suggested change
-indicate
-utilization📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| indicate | |
| utilization |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/vale/styles/config/vocabularies/Elements/accept.txt` around lines 361
- 362, The Elements vocabulary entry currently includes generic terms that
should not be part of the shared docs vocabulary. Remove indicate and
utilization from the accept list in the Elements vocabulary file, and keep this
vocabulary limited to technical terms, component names, or abbreviations that
are specific to the project.
Source: Coding guidelines
Comment on lines
+4
to
+21
| :host { | ||
| --color: var(--nve-sys-text-emphasis-color); | ||
| --background: var(--nve-sys-interaction-background); | ||
| --accent-color: var(--nve-sys-interaction-color); | ||
| --gauge-width: 12px; | ||
| --font-size: var(--nve-ref-font-size-400); | ||
| --gap: var(--nve-ref-space-xs); | ||
| --width: 128px; | ||
| --height: var(--width); | ||
| --_animation-duration: var(--nve-ref-animation-duration-250); | ||
| display: inline-block; | ||
| position: relative; | ||
| width: var(--width); | ||
| height: var(--height); | ||
| container-type: inline-size; | ||
| contain: content; | ||
| text-box: trim-both cap alphabetic; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix the Stylelint failure in :host.
This block is missing the required blank line before display, so it currently fails declaration-empty-line-before.
Suggested fix
--height: var(--height);
--_animation-duration: var(--nve-ref-animation-duration-250);
+
display: inline-block;
position: relative;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| :host { | |
| --color: var(--nve-sys-text-emphasis-color); | |
| --background: var(--nve-sys-interaction-background); | |
| --accent-color: var(--nve-sys-interaction-color); | |
| --gauge-width: 12px; | |
| --font-size: var(--nve-ref-font-size-400); | |
| --gap: var(--nve-ref-space-xs); | |
| --width: 128px; | |
| --height: var(--width); | |
| --_animation-duration: var(--nve-ref-animation-duration-250); | |
| display: inline-block; | |
| position: relative; | |
| width: var(--width); | |
| height: var(--height); | |
| container-type: inline-size; | |
| contain: content; | |
| text-box: trim-both cap alphabetic; | |
| } | |
| :host { | |
| --color: var(--nve-sys-text-emphasis-color); | |
| --background: var(--nve-sys-interaction-background); | |
| --accent-color: var(--nve-sys-interaction-color); | |
| --gauge-width: 12px; | |
| --font-size: var(--nve-ref-font-size-400); | |
| --gap: var(--nve-ref-space-xs); | |
| --width: 128px; | |
| --height: var(--width); | |
| --_animation-duration: var(--nve-ref-animation-duration-250); | |
| display: inline-block; | |
| position: relative; | |
| width: var(--width); | |
| height: var(--height); | |
| container-type: inline-size; | |
| contain: content; | |
| text-box: trim-both cap alphabetic; | |
| } |
🧰 Tools
🪛 Stylelint (17.13.0)
[error] 14-14: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.css` around lines 4 - 21, The
:host rule in progress-gauge.css fails declaration-empty-line-before because
there is no blank line before display; update the :host block to add the
required empty line separating the custom property declarations from the
standard declarations, keeping the fix within the progress-gauge styles.
Source: Linters/SAST tools
Comment on lines
+114
to
+123
| <script type="module"> | ||
| const gauge = document.querySelector('#dynamic-gpu-progress-gauge'); | ||
| const valueElement = gauge.querySelector('span'); | ||
| setInterval(() => { | ||
| const value = Math.floor(Math.random() * 101); | ||
| gauge.value = value; | ||
| gauge.status = value >= 80 ? 'danger' : value >= 60 ? 'warning' : 'success'; | ||
| valueElement.textContent = value + '%'; | ||
| }, 1500); | ||
| </script> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Scope and tear down the Dynamic example timer.
The inline module does a document-global lookup and creates an unmanaged setInterval(). Re-rendering this example can stack timers and keep updating the wrong or a detached gauge, which makes docs and visual coverage flaky.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.examples.ts` around lines 114
- 123, The Dynamic example script creates an unmanaged interval and uses a
document-wide query, which can stack timers and keep mutating a detached or
wrong gauge on rerender. Update the inline module in progress-gauge.examples.ts
to scope lookups from the example’s own root container instead of document, and
keep a handle to the timer created in the dynamic gauge example. Add
teardown/cleanup for that setInterval when the example is re-rendered or
disposed so only one timer runs per instance.
Comment on lines
+15
to
+24
| beforeEach(async () => { | ||
| fixture = await createFixture(html` | ||
| <nve-progress-gauge aria-label="progress" value="0"></nve-progress-gauge> | ||
| <nve-progress-gauge aria-label="progress" value="50"></nve-progress-gauge> | ||
| <nve-progress-gauge aria-label="progress" status="warning" value="75"></nve-progress-gauge> | ||
| <nve-progress-gauge aria-label="progress" container="half" status="success" value="100"></nve-progress-gauge> | ||
| `); | ||
| element = fixture.querySelector(ProgressGauge.metadata.tag); | ||
| await elementIsStable(element); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wait for every rendered gauge before running axe.
beforeEach only stabilizes the first <nve-progress-gauge>. The other three instances can still be updating when runAxe() scans the page, which makes this test flaky and can hide violations on their settled DOM.
Suggested fix
beforeEach(async () => {
fixture = await createFixture(html`
<nve-progress-gauge aria-label="progress" value="0"></nve-progress-gauge>
<nve-progress-gauge aria-label="progress" value="50"></nve-progress-gauge>
<nve-progress-gauge aria-label="progress" status="warning" value="75"></nve-progress-gauge>
<nve-progress-gauge aria-label="progress" container="half" status="success" value="100"></nve-progress-gauge>
`);
- element = fixture.querySelector(ProgressGauge.metadata.tag);
- await elementIsStable(element);
+ const gauges = Array.from(
+ fixture.querySelectorAll(ProgressGauge.metadata.tag),
+ ) as ProgressGauge[];
+ [element] = gauges;
+ await Promise.all(gauges.map(gauge => elementIsStable(gauge)));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(async () => { | |
| fixture = await createFixture(html` | |
| <nve-progress-gauge aria-label="progress" value="0"></nve-progress-gauge> | |
| <nve-progress-gauge aria-label="progress" value="50"></nve-progress-gauge> | |
| <nve-progress-gauge aria-label="progress" status="warning" value="75"></nve-progress-gauge> | |
| <nve-progress-gauge aria-label="progress" container="half" status="success" value="100"></nve-progress-gauge> | |
| `); | |
| element = fixture.querySelector(ProgressGauge.metadata.tag); | |
| await elementIsStable(element); | |
| }); | |
| beforeEach(async () => { | |
| fixture = await createFixture(html` | |
| <nve-progress-gauge aria-label="progress" value="0"></nve-progress-gauge> | |
| <nve-progress-gauge aria-label="progress" value="50"></nve-progress-gauge> | |
| <nve-progress-gauge aria-label="progress" status="warning" value="75"></nve-progress-gauge> | |
| <nve-progress-gauge aria-label="progress" container="half" status="success" value="100"></nve-progress-gauge> | |
| `); | |
| const gauges = Array.from( | |
| fixture.querySelectorAll(ProgressGauge.metadata.tag), | |
| ) as ProgressGauge[]; | |
| [element] = gauges; | |
| await Promise.all(gauges.map(gauge => elementIsStable(gauge))); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.test.axe.ts` around lines 15
- 24, The axe test setup only waits for the first nve-progress-gauge to
stabilize, so the remaining gauges may still be rendering when runAxe executes.
Update the beforeEach in progress-gauge.test.axe.ts to wait for every rendered
ProgressGauge instance from the fixture, not just the element returned by
querySelector, so the DOM is fully settled before accessibility checks run.
Comment on lines
+19
to
+24
| function template(theme: '' | 'dark' = '') { | ||
| return /* html */ ` | ||
| <script type="module"> | ||
| import '@nvidia-elements/core/progress-gauge/define.js'; | ||
| document.documentElement.setAttribute('nve-theme', '${theme}'); | ||
| </script> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't set nve-theme on the default baseline.
When theme === '', this still adds an empty nve-theme attribute. That means the “default” snapshot is no longer exercising the actual default document state, and any selectors that depend on attribute presence can produce a false baseline.
Suggested fix
function template(theme: '' | 'dark' = '') {
return /* html */ `
<script type="module">
import '`@nvidia-elements/core/progress-gauge/define.js`';
- document.documentElement.setAttribute('nve-theme', '${theme}');
+ if ('${theme}') {
+ document.documentElement.setAttribute('nve-theme', '${theme}');
+ } else {
+ document.documentElement.removeAttribute('nve-theme');
+ }
</script>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function template(theme: '' | 'dark' = '') { | |
| return /* html */ ` | |
| <script type="module"> | |
| import '@nvidia-elements/core/progress-gauge/define.js'; | |
| document.documentElement.setAttribute('nve-theme', '${theme}'); | |
| </script> | |
| function template(theme: '' | 'dark' = '') { | |
| return /* html */ ` | |
| <script type="module"> | |
| import '`@nvidia-elements/core/progress-gauge/define.js`'; | |
| if ('${theme}') { | |
| document.documentElement.setAttribute('nve-theme', '${theme}'); | |
| } else { | |
| document.documentElement.removeAttribute('nve-theme'); | |
| } | |
| </script> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.test.visual.ts` around lines
19 - 24, The visual test template is always setting the nve-theme attribute,
even for the default baseline when theme is empty. Update template to only call
document.documentElement.setAttribute in the dark-theme case, and leave the
default document state untouched when theme === ''. Use the template helper in
progress-gauge.test.visual.ts as the place to guard the attribute assignment.
Comment on lines
+22
to
+26
| /** | ||
| * @element nve-progress-gauge | ||
| * @description Use progress gauge to indicate system resource utilization. | ||
| * @since 0.0.0 | ||
| * @entrypoint \@nvidia-elements/core/progress-gauge |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Replace the 0.0.0 placeholders in the public version metadata.
@since and ProgressGauge.metadata.version still advertise 0.0.0, so generated docs and CLI/MCP consumers will expose the wrong release information for this component.
Suggested fix
- * `@since` 0.0.0
+ * `@since` 2.0.2
@@
static readonly metadata = {
tag: 'nve-progress-gauge',
- version: '0.0.0'
+ version: '2.0.2'
};Also applies to: 41-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.ts` around lines 22 - 26, The
public version metadata for ProgressGauge still uses a placeholder release
value. Update the `@since` tag in the component docblock and the
ProgressGauge.metadata.version field so they reflect the real published version
instead of 0.0.0, and make the same change anywhere else in this diff that
duplicates the placeholder metadata.
Comment on lines
+49
to
+53
| /** The current `value` of the progress gauge. */ | ||
| @property({ type: Number }) value = 0; | ||
|
|
||
| /** The `max` value of the progress gauge that the `value` is proportionally scaled to. */ | ||
| @property({ type: Number }) max? = 100; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize value and max before using them for geometry and ARIA.
max=0, negative inputs, NaN, or value > max currently produce invalid dash arrays and out-of-range aria-valuenow/aria-valuemax pairs. Clamp once and reuse the normalized numbers in both render() and updated().
Suggested fix
+ `#normalizedState`() {
+ const max = Number.isFinite(this.max) && this.max! > 0 ? this.max! : 100;
+ const value = Number.isFinite(this.value)
+ ? Math.min(Math.max(this.value, 0), max)
+ : 0;
+
+ return { value, max, progress: (value / max) * 100 };
+ }
+
render() {
const geometry = GAUGE_GEOMETRY[this.container ?? 'default'];
- const progress = (this.value / (this.max ?? 100)) * 100;
+ const { value, progress } = this.#normalizedState();
return html`
<div internal-host>
<svg viewBox=${geometry.viewBox} role="presentation" aria-hidden="true">
<path pathLength="100" d=${geometry.path} class="background"></path>
<path pathLength="100" d=${geometry.path} class="gauge"
- ?empty=${progress <= 0}
+ ?empty=${value <= 0}
style=${`--_progress: ${progress};`}
stroke-dasharray=${`${progress} 100`}>
</path>
</svg>
<slot></slot>
</div>
`;
}
updated(props: PropertyValues<this>) {
super.updated(props);
- this._internals.ariaValueNow = `${this.value}`;
- this._internals.ariaValueMax = `${this.max}`;
+ const { value, max } = this.#normalizedState();
+ this._internals.ariaValueNow = `${value}`;
+ this._internals.ariaValueMax = `${max}`;As per coding guidelines, component properties/attributes should avoid impossible states.
Also applies to: 69-80, 94-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.ts` around lines 49 - 53,
Normalize the progress gauge inputs before they are used in geometry or ARIA: in
progress-gauge.ts, clamp and sanitize `value` and `max` so `NaN`, negative
values, `max <= 0`, and `value > max` cannot flow into rendering. Update
`render()` and `updated()` to reuse the same normalized numbers when computing
the dash geometry and setting `aria-valuenow`/`aria-valuemax`, using the `value`
and `max` properties as the source symbols to locate the fix.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes