fix(scorecards): correct check output units and related properties - #60
Conversation
Two defects in how check results render, both found by comparing the plugin against DX's own scorecard UI for the same entity. Custom output units were pluralized before display, but the unit is author-supplied and already written in its intended form. `pluralize()` appends "s" to anything outside its special-case list, so units came out doubled: "4 trace metricss", "89 charss". Render the unit verbatim, as DX does. `pluralize()` still serves the duration_* output types, whose units are hardcoded singulars. The related-property section never rendered for any check. The API returns `related_properties` as an array; the type and the drawer both expected a singular `related_property` string, which no response contains. Correct the field and render every property in the array. Also fixes `CustomOutputOptions.unit`, typed as the string literal "string" rather than `string`.
The chip set a light background but left the text color inherited, so dark themes rendered near-white text on it at a 1.1:1 contrast ratio. Pair the background with an explicit foreground, as the check result badges already do.
|
Hi @colmcahalane-toast, thanks for the contribution! I'm reviewing and QAing now. |
|
|
||
| export type CustomOutputOptions = { | ||
| unit: "string"; | ||
| unit: string; |
There was a problem hiding this comment.
Thanks for cleaning this one up 😳
There was a problem hiding this comment.
This library's pluralize function has definitely diverged from the app. Sorry about that!
But heads up that this change creates a regression. If the unit is defined in the app as a singular value like "widget", the component correctly renders 1 widget or 2 widgets on the main branch, but 1 widget and 2 widget on this PR's branch.
I don't want to keep you waiting or sign you up for more work, so I'm going to merge this PR as written, then copy in the newer pluralize function we have defined in the app, which should handle both singular and plural cases correctly.
2 participants
Two rendering defects in scorecard check results, found by comparing the plugin's output against the DX web UI for the same entity, plus a follow-on contrast fix.
Custom output units were pluralised twice
formatCustomOutputValuepassed the author-suppliedunitthroughpluralize(), which appendssto anything outside its special-case list. Units are already written in their intended form, so they came out doubled:trace metrics4 trace metricss4 trace metricschars89 charss89 charsmonitors71 monitorss71 monitors% time > 90% util0.00 % time > 90% utils0.00 % time > 90% utilscreenshot taken with different data,
count difference expected
The unit is now rendered verbatim, matching how DX's own UI displays it.
pluralize()still serves theduration_*output types, whose units are hardcoded singulars (second,minute, …), so1 day/3 daysbehaviour is unchanged.The related-property section never rendered
entities.scorecardsreturnsrelated_propertiesas an array of property identifiers. The type and the drawer both expected a singularrelated_propertystring, which no response contains — so the "Related property / Edit in DX" block was dead code and never appeared for any check, on any entity.Verified against a live API response: of 18 checks on one scorecard, 9 carried a
related_propertiesvalue and none had arelated_propertykey. The field is now correct and every entry in the array is rendered, with the label switching between singular and plural on count.Related-property chips were illegible on dark themes
Once the section above started rendering, the chip turned out to set a light
background-colorwhile leavingcolorinherited — near-white text on#F3F4F6, a contrast ratio of 1.1:1 under a dark theme. It now pairs the background with an explicit foreground, asCheckResultBadgealready does for its status colours, giving 9.4:1.Also included
CustomOutputOptions.unitwas typed as the string literal"string"rather thanstring, making any real unit value a type error.CheckResultBadgeandCheckResultDrawercovering the above. These are the first component tests in the package; each one was confirmed to fail against the unfixed code.Testing
yarn tsc,yarn lint,yarn format:check,yarn testandyarn buildall pass. Also verified in a real Backstage instance (Backstage 1.51, dark theme) by installing the packed tarball, where the badge text and the related-property row both render as expected.These changes were written with Claude Code and reviewed by me before submitting.