feat: implement dynamic score color for toolbar health indicators - #255
feat: implement dynamic score color for toolbar health indicators#255dermatz wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, consistent with existing DOM structure/selectors, and apply styling in a self-contained way without altering score calculation logic.
Pull request overview
This PR adds dynamic colour styling for MageForge toolbar “health score” indicators so scores visually communicate severity (low/mid/high) across the home gauge, audit panel rings, and dashboard category badges.
Changes:
- Introduces a shared
_applyScoreColorClass()helper to consistently applymageforge-score--low|mid|highbased on score thresholds. - Updates score rendering to apply colour classes to the appropriate wrapper elements while still updating the numeric text.
- Adds CSS rules mapping the score classes to the existing MageForge colour variables (red/yellow/green).
File summaries
| File | Description |
|---|---|
| src/view/frontend/web/js/toolbar/ui/score.js | Applies score-based CSS classes while updating the displayed score values across toolbar widgets. |
| src/view/frontend/web/css/toolbar/_health.css | Defines the dynamic score colour styles for low/mid/high score states using MageForge CSS variables. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The new mid-score colour uses --mageforge-color-yellow instead of the toolbar’s established --mageforge-color-amber warning/mid token, creating an inconsistent palette.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/view/frontend/web/css/toolbar/_health.css:145
- The mid-score colour uses
--mageforge-color-yellow, but elsewhere the toolbar uses--mageforge-color-amberfor warning/mid states (e.g.toolbar/_findings.css:58-62), and the doc comment above describes the mid range as “amber”. Using yellow here makes the palette inconsistent.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new “mid” score styling uses --mageforge-color-yellow which conflicts with the toolbar’s established amber warning/mid palette, causing inconsistent severity colouring.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Confirmed logic issues in score colour application (NaN handling and missing colour application for ring widgets) should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/view/frontend/web/js/toolbar/ui/score.js:31
- _applyScoreColorClass treats NaN as a valid score (all comparisons are false, so it falls into the “high” branch). This can incorrectly colour the UI green if an upstream calculation yields NaN/undefined.
src/view/frontend/web/js/toolbar/ui/build.js:120 - The PR title is scoped to “dynamic score color”, but this change set also adds a theme toggle UI in the header and refactors the footer/export controls. Consider either splitting these UI changes into a separate PR or updating the PR title/description to reflect the additional behaviour so reviewers can assess scope and risk appropriately.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
| this.menu | ||
| .querySelectorAll(".mageforge-toolbar-health-score-number") | ||
| .querySelectorAll(".mageforge-toolbar-health-score-value") | ||
| .forEach((el) => { | ||
| el.textContent = score; | ||
| const number = el.querySelector( | ||
| ".mageforge-toolbar-health-score-number", |
No description provided.