Repository navigation
Conversation
mudabs
left a comment
There was a problem hiding this comment.
Thank you, Moon, for building these reusable health UI components and adding focused tests.
Please address the following before approval:
-
Reuse the existing semantic status helper at
frontend/src/utils/healthStatusClasses.jsinstead of duplicating raw Tailwind color mappings inHealthBadge.jsx. -
Fix
HealthMetricCard.jsxaccessibility. The currentaria-label={title}hides the metric value, unit, status, and description from screen readers. Remove it or provide properaria-labelledbyandaria-describedbyrelationships so the complete metric is announced. -
Improve
HealthStateView.jsxsemantics:- Give the region an accessible name linked to its title.
- Use an assertive alert treatment for error states.
- Make the loading indicator label configurable instead of hard-coding
Loading content.
-
Add tests for:
- Space key activation in addition to Enter.
- Accessible metric value, unit, status, and description.
- Loading, empty, and error announcements.
- Unknown status fallback.
- Localized loading text in at least English and Arabic.
-
Confirm whether these components are intended to remain unused until a later PR. If they are part of the current UI work, integrate at least one into an existing health route.
Please run and pass:
npm run lintnpm run test:runnpm run build
Then push the updates and confirm that GitHub Actions checks pass.
…s, assertive alerts, and Vitest coverage
|
I have resolved all requested feedback and resolved the branch conflict with upstream
|
mudabs
left a comment
There was a problem hiding this comment.
Thank you Moon for addressing the semantic status helper, accessibility relationships, configurable loading labels, assertive error states, and expanded tests.
Please address these items before merge:
- Fix the failing critical-status test in
frontend/src/components/HealthUIComponents.test.jsx. It expects aroseclass, but the shared helper correctly returnsborder-health-critical bg-health-critical/10 text-red-950. Update the assertion to validate the shared semantic classes. - Integrate at least one reusable component into an existing health route, or clearly document that this is a foundation-only PR and link the follow-up integration work. The new components are currently used only by their tests.
- Re-run
npm run lint,npm run test:run, andnpm run build, then confirm Node 20, Node 22, and Build all pass.
…lper exports, and test assertions (oss-slu#158)
…lper exports, and test assertions (oss-slu#158)
|
|
I have updated the PR to address your latest review feedback:
|



Summary
Changes Made
frontend/src/components/HealthBadge.jsx:frontend/src/components/HealthMetricCard.jsx:frontend/src/components/HealthStateView.jsx:frontend/src/components/HealthUIComponents.test.jsx:Testing Details
npm run test:run), successfully passing all 9 test suites and 113 tests with zero regressions.npm run lint), passing with zero warnings or errors.Checklist
Closes #158