Repository navigation
feat(charts): add MetricTile - #10043
nbozidarevic wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
There was a problem hiding this comment.
Outcome: approval waits on an engineering owner's spec decision, and the fixes below are needed either way.
No current spec governs MetricTile. An engineering owner needs to provide or identify its governing record and settle four things: whether it ships as a new component or comes from promoting Lab Stat; its public names (MetricTile, trend, sentiment, size); the default delta colours and the sentiment override; and the screen-reader strings. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Required fixes:
MetricTile.stories.tsximportsSparklinefrom@astryxdesign/charts, which does not export it, so all three MetricTile stories fail to load and the typecheck fails; render the trend slot without Sparkline or stack this change on the Sparkline pull request.- The doc example in
MetricTile.doc.mjsand the JSDoc example inMetricTile.tsximport the same missingSparkline, so code copied from them does not compile. .changeset/charts-metrictile.mdfailspnpm check:changesets(the charts package is private,minordoes not match[feat]while 0.x, and the body has no@contributorline); remove it, since the core changeset already covers the new strings.MetricTileemits the theme targetastryx-metric-tilewith asizeaxis, butMetricTile.doc.mjsdeclares notheming.targets, which the component theming-surface architecture treats as a defect; add the target metadata or dropthemeProps.MetricTile.tsxrepeats LabStatline for line, so a fix to one (like the translated strings added here) misses the other; keep one implementation by promotingStatunder the name the owners choose.
CI has not run on this head yet: a maintainer adds the ci:run label so it runs.
[Automated review]
8549ff7 to
b9f7181
Compare
|
Sweep update: addressed the actionable threads in b9f7181 (rebased on current main) — bad changeset deleted, trend slot now renders with the shipped One thread left open deliberately: the Lab |
There was a problem hiding this comment.
Outcome: approval waits on an engineering owner's spec decision, and the fixes below are needed either way.
No current spec governs MetricTile, so an engineering owner needs to provide or identify its governing record and settle whether it ships as a new component or comes from promoting Lab Stat, its public names (MetricTile, trend, sentiment, size and the astryx-metric-tile theme target), the default delta colours with the sentiment override, and the screen-reader strings. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Required fixes:
- The trend example in
MetricTile.stories.tsx,MetricTile.doc.mjsand theMetricTile.tsxJSDoc renders no line, becauseChartatheight={48}keeps its default 24 px top and 32 px bottom margins and leaves a 0 px plot; pass a smallmargin(for example{top: 4, right: 0, bottom: 4, left: 0}) and decide whether the tile should show the visibletitleheading. astryx-metric-tileis the first theme target in@astryxdesign/charts, but the consistency guard inpackages/core/src/theme/themingTargets.test.tsstill scans only core, Lab and richtext; enrolpackages/charts/srcin it, as the theming-surface architecture requires when a package starts participating.MetricTile.tsxrepeats LabStatline for line while LabStatstill hard-codes English(trending up); once an owner rules, keep one implementation by promotingStator by removing it the way Chart moved to charts.- Miss: the doc prose and source comments still describe the trend slot as "for a Sparkline" (
MetricTile.doc.mjs:25,:90,:124;MetricTile.tsx:7,:176,:187), which no package exports; describe it as a small trend chart, such as aChartwith onelinemark.
CI has not run on this head yet: a maintainer adds the ci:run label so it runs.
[Automated review]
|
Paused: per the discussion in #8860, new chart components need approved specs before they can land, and the chart API questions are still open. Leaving this PR open as-is (not closing) until its spec exists; no further iteration planned meanwhile. |
b9f7181 to
f93c8f1
Compare
There was a problem hiding this comment.
Outcome: approval waits on an engineering owner's spec decision, and the fixes below are needed either way.
No current spec governs MetricTile, so an engineering owner needs to provide or identify its governing record and settle whether it ships as a new component or comes from promoting Lab Stat, its public names (MetricTile, trend, sentiment, size and the astryx-metric-tile theme target), the default delta colours with the sentiment override, and the screen-reader strings. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Required fixes:
- The trend examples in
MetricTile.doc.mjsand theMetricTile.tsxJSDoc still giveChartheight={48}with its default margin, which leaves a 0 px plot and no line; pass the story'smargin={{top: 4, right: 0, bottom: 4, left: 0}}in both. - Miss:
apps/storybook/rtl-audit/verified-not-applicable.jsonsays MetricTile owns no inline-axis order, but its delta swaps sides with the value under RTL, the pair LabStatis audited for, so replace the entry with a D2 layout-order target intargets.jsonand update the coverage test. MetricTile.tsxrepeats LabStatline for line while LabStatstill hard-codes English(trending up); once an owner rules, keep one implementation by promotingStator by removing it the way Chart moved to charts.
CI has not run on this head yet: a maintainer adds the ci:run label so it runs.
[Automated review]
f93c8f1 to
b7382a8
Compare
There was a problem hiding this comment.
Outcome: approval waits on an engineering owner's spec decision, and one fix is needed either way.
No current spec governs MetricTile, so an engineering owner needs to provide or identify its governing record and settle whether it ships as a new component or comes from promoting Lab Stat, its public names (MetricTile, trend, sentiment, size and the astryx-metric-tile theme target), the default delta colours with the sentiment override, and the screen-reader strings. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Required fix:
MetricTile.tsxrepeats LabStatline for line whileStatstill hard-codes English(trending up), so once an owner rules, keep one implementation by promotingStatunder the chosen name or by removing it from Lab the way Chart moved to charts.
CI has not run on this head yet: a maintainer adds the ci:run label so it runs.
[Automated review]
KPI tile with sentiment-aware delta and a trend slot. Split from the charts bundle so each chart ships separately.
Delete the private-package changeset. Render the trend slot with the shipped Chart and line mark instead of the unlanded Sparkline in the story, doc example, and JSDoc example. Declare the astryx-metric-tile theme target in the component doc. Register MetricTile in the CLI official-packages bundle and the pinned component lists, and classify it for the RTL audit.
Give the story trend chart a small margin so the line is not clipped at 48px height, describe the trend slot as a small trend chart instead of the not-yet-existing Sparkline, and enrol packages/charts in the theming-targets guard with a ChartSwatchImpl variant exception.
Problem: four review threads were left open on the MetricTile change, and the branch had drifted from main (theming guard conflict). Fix: pass the story's small Chart margin in the doc and JSDoc trend examples so the 48px-tall plot draws; replace the MetricTile verified-not-applicable entry with a measured D2 RTL target on the inverted-delta story and update the audit coverage fixture; rebase onto current main, keeping main's theming guard text and dropping the now stale Code.size exception the rebase surfaced. The Lab Stat duplication thread is intentionally left for the spec owner, with no code change. Testing: vitest MetricTile (12 passed), themingTargets (459 passed), rtl-audit-coverage (81 passed); RTL target sort check; eslint clean on touched sources.
b7382a8 to
8f11f7f
Compare
There was a problem hiding this comment.
Outcome: approval waits on an engineering owner's spec decision, and one fix is needed either way.
No current spec governs MetricTile. An engineering owner needs to provide or identify its governing record and settle whether it ships as a new component or comes from promoting Lab Stat, its public names (MetricTile, trend, sentiment, size and the astryx-metric-tile theme target), the default delta colours with the sentiment override, and the screen-reader strings. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Required fix:
MetricTile.tsxrepeats LabStatline for line whileStatstill hard-codes English(trending up), so once an owner rules, keep one implementation by promotingStatunder the chosen name or by removing it from Lab the way Chart moved to charts.
CI has not run on this head yet: a maintainer adds the ci:run label so it runs.
[Automated review]
Split from #8860 so each chart ships separately.
MetricTile: a KPI tile with sentiment-aware delta and a trend slot (pair with Sparkline via
trend).Stories: MetricTile. Note: the MetricTile suite crashes its vitest worker at import on my dev box, identically on the clean tree (environment issue) — leaving verification to CI.
Still pending (as on the bundle): governing specs and spec-owner approval (including whether MetricTile should come from promoting the lab Stat component instead), design review, and a
ci:runlabel for CI. Merging will also want a look at the adjacentindex.tsexport lines the sibling chart PRs add.