Repository navigation
feat(charts)!: draw charts like a report's, with heatmaps, units, and log scales - #406
Conversation
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
comark-annotate
comark-badge
comark-cards
comark-carousel
comark-cast
comark-charts
comark-chat
comark-chips
comark-cjk
comark-code
comark-code-notes
comark-colors
comark-comments
comark-critic
comark-deflist
comark-details
comark-diagrams
comark-diff
comark-embed
comark-fields
comark-figure
comark-file-tree
comark-footnotes
comark-github
comark-icon
comark-include
comark-keys
comark-links
comark-linter
comark-magic-move
comark-mark
markdown-it-to-comark
marked-to-comark
comark-mathml
comark-media
comark-metrics
migrate-to-comark
comark-outline
comark-properties
comark-qr
comark-quiz
comark-reading-time
remark-to-comark
comark-sources
comark-steps
comark-table
comark-tabs
comark-tests
comark-time
comark-ui
comark-xref
comark-zoom
commit: |
Latest result Pull request merged. Final answersSession Pull request merged. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. Session Updated the branch with main because main changed skills/report/SKILL.md after its checks ran; waiting for checks on the new head. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Reproducible defects remain in log-scale validation, line labels, waterfall units, and heatmap text fitting, despite the updated head passing the existing focused tests.
Reviewed changes — Reviewed the report-style chart changes and the follow-up layout fix through 766cf40.
- Report presentation: Charts become unframed sections with descriptions, a muted palette, dotted grids, default bar labels, and revised horizontal spacing.
- Chart capabilities: Adds heatmaps with directional scoring, compact values with units, logarithmic axes, and direct labels at line ends.
- Renderer and authoring contracts: Updates HTML, Vue, React, and Svelte output, adds three lint rules and Doctor examples, and updates documentation and agent skills.
- Metric tiles and browser coverage: Converts statistic tiles into a wrapping strip and adjusts the chart browser contract for the hairline presentation.
Validation: pnpm build, 381 focused tests, and all six chart E2E tests pass at the reviewed head. Additional public-interface and browser reproductions demonstrate the inline findings.
gpt-6.1-sol(xhigh) ｜ 𝕏
There was a problem hiding this comment.
Important
No new defects found in fd9664a; approval remains withheld because the prior review's P1 findings remain unresolved.
Reviewed changes — Reviewed the CSS changes since the previous review at 766cf40.
- Removed redundant chart dividers. Charts immediately after plain or wrapped headings now omit their top hairline and padding, including the first HTML-rendered chart after its inline styles and script.
- Reduced statistic tile width. Changed the flex basis from
8remto7rem, fitting seven tiles into an 800px strip while preserving wrapping in narrower columns.
Validation: pnpm build and all 72 chart/metrics tests passed. Chromium checks confirmed heading-specific spacing, retained dividers on other charts, and tile wrapping without container overflow.
gpt-6.1-sol(xhigh) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found; no P0 or P1 findings remain.
Reviewed changes — Reviewed cb63d5c, the fixes since the previous Pullfrog review at fd9664a.
- Validated effective log axes. Continued positivity checks after categorical-x fallback, guarded log-tick bounds, and rejected nonpositive reference targets on logarithmic value axes.
- Aligned line names with rendered geometry. Derived label and leader colors from line strokes and selected each series’ rightmost rendered endpoint.
- Applied waterfall unit formatting. Formatted computed-axis ticks, totals, and signed changes consistently, including the width checks that decide whether labels fit.
- Suppressed oversized heatmap text. Left values that cannot fit their cells in the Data table and added public parse-and-render regressions for the repaired behavior.
Validation: pnpm build, all 391 selected tests, all six chart E2E tests, and 19 additional public-interface probes passed. The six previously reported defects are addressed, and their threads were already resolved.
gpt-6.1-sol(xhigh) ｜ 𝕏
… log scales
A chart is now a section under a hairline instead of a card: its title and
a new `description` at the start, values printed on bars by default, lines
named where they end in their colors, points on every value, dotted grid
lines, thinner bars, and a muted palette. `type="heatmap"` draws one table
against another, `better` colors its cells red to green, `unit` prints
numbers as `$22.2k`, and `log` puts an axis on a log scale.
`::stats{tiles}` is now one strip of figures with hairlines between them.
BREAKING CHANGE: charts drop their card and `data-comark-frame`,
`--cch-padding` defaults to 0, bars and heatmaps print their numbers unless
`labels=false`, numbers from a thousand up print shortened, and the series
palette changed.
… no label touches another
…even stat tiles in one strip
…umbers past their ends Every chart over axes is drawn 744px wide and scales to the column, so all charts on a page match. Upright bars stay about 40px wide, plain bars print their number past their end, axis text is 11px, lines are 1.5px, and the value axis's name reads across its top. The playground gains Heatmap, Scores, Money, and Log scale presets.
…s over eight steps
cb63d5c to
5548820
Compare
There was a problem hiding this comment.
Important
The new fixed-width layout can collapse a line plot to zero width and overestimate the room available for waterfall labels.
Reviewed changes — Reviewed the changes since the previous Pullfrog review at cb63d5c, through 5548820.
- Standardized chart sizing. Drew axis-based charts at 744px including padding and scaled unmarked SVGs to fill their column.
- Refined marks and labels. Slimmed upright bars, moved ordinary bar values outside their ends, reduced text and stroke sizes, and adjusted axis titles and crowded waterfall labels.
- Expanded examples and checks. Added heatmap, score, money, and log-scale examples, explicitly narrowed validated log-axis types, and expanded the waterfall unit-fitting fixture to eight steps.
Validation: pnpm build, pnpm typecheck, the chart package’s Doctor check, all 391 selected tests, and all six chart E2E tests pass. Additional public parse-and-render comparisons and Chromium measurements reproduce the two inline findings.
gpt-6.1-sol(xhigh) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found; no P0 or P1 findings remain.
Reviewed changes — Reviewed 9bbb227, the fixes since the previous Pullfrog review at 5548820.
- Preserved line plotting space. Switched long series names to a width-limited legend while retaining direct end labels for shorter names.
- Fitted waterfall values to rendered spacing. Checked the final x-scale positions after axis fitting and band padding, then suppressed value labels that would overlap.
- Added regression coverage and documentation. Covered the reported long-header chart and three/seven-step waterfall geometry, and documented the legend fallback.
Validation: the workspace build, typecheck, chart package’s Doctor check, all 394 selected tests, and all six chart E2E tests pass. Another 72 public parse-and-render fixtures passed; Chromium measurements found no clipped or overlapping SVG text at 720px and no chart overflow at 360px. Both previous findings are addressed, and their threads were already resolved.
gpt-6.1-sol(xhigh) ｜ 𝕏
…' color when its first cell is empty
…margin, so later bars keep their numbers
There was a problem hiding this comment.
Important
The new label placement drops series identity from ordinary bar values, causing series-specific annotation outlines to cross their labels. A separate date-normalization edge case prints an incorrect stack total.
Reviewed changes — Reviewed the substantive changes in 9c301d4 after the previously checked e45e81a.
- Replaced outside segment values with stack totals. Kept fitting nonzero segment labels inside, added sign-separated totals outside, and reserved horizontal space for their formatted values.
- Pinned series colors to table columns. Preserved header-order color and legend mappings when a series begins with an empty cell.
- Updated diagnostics and coverage. Restricted the additive log-scale explanation to additive chart types and updated documentation and regression tests.
Validation: the workspace build, typecheck, chart Doctor check, all 395 selected tests, and all six chart E2E tests pass. Additional public parse-and-render probes and Chromium comparisons reproduce the findings below.
gpt-6.1-sol(xhigh) ｜ 𝕏
There was a problem hiding this comment.
Important
No new defects found in 1b79d4a. Approval remains withheld because the previous review’s P1 finding remains unresolved.
Reviewed changes — Reviewed the label-collision fix since the previous Pullfrog review at 9c301d4.
- Corrected collision-check arguments. Wrapped
covers()soArray.some()no longer supplies its index as the spacing margin, preventing later bar values from being incorrectly suppressed. - Adjusted label clearance. Set a consistent 6px margin between estimated value-label bounds and other labels.
- Added regression coverage. Verified that all 30 values remain present in a horizontal bar chart.
Validation: the workspace build, typecheck, changed-file formatting/lint checks, chart Doctor check, 491 selected tests, all six chart E2E tests, and eight additional public parse-and-render checks passed; one unrelated test was skipped. Both previously open findings remain unchanged, so their existing threads were left open without duplicate comments.
gpt-6.1-sol(xhigh) ｜ 𝕏
There was a problem hiding this comment.
Important
The identity fixes are in place, but three newly added regression tests fail at this head and block the required test suite.
Reviewed changes — Reviewed dd1c2a0, the changes since the previous Pullfrog review at 1b79d4a.
- Preserved ordinary label identity. Retained each bar value's series while clearing it only on synthetic stack totals.
- Aggregated rendered stacks. Grouped totals by rendered lane and sign so equivalent timestamp spellings share the correct aggregate.
- Expanded regression coverage. Added positive and negative annotation checks and positive, negative, and mixed-sign timestamp-stack cases in both orientations.
Validation: the workspace build, typecheck, chart Doctor check, and all six chart browser tests pass. The selected chart, metrics, and Doctor tests report 397 passed and 3 failed; running the regression file alone reproduces the same three failures.
gpt-6.1-sol(xhigh) ｜ 𝕏
There was a problem hiding this comment.
Important
The previous regression failures are fixed, but the new padding branch fails the required Doctor check.
Reviewed changes — Reviewed ffc21af, the changes since the previous Pullfrog review at dd1c2a0.
- Aligned annotation coordinates. Used the rendered view’s padding instead of fixed offsets when locating chart marks.
- Preserved negative-stack coverage. Tested both permitted label suppression at the axis and correct negative aggregates when a larger control stack leaves room.
- Formatted regression fixtures. Reformatted the affected parameterized tests without dropping their negative-input cases.
Validation: the workspace build, typecheck, changed-file checks, all 400 selected tests, all six chart browser tests, and 20 additional annotation-geometry fixtures pass. The chart Doctor check fails reproducibly with TS0007 at the new branch.
gpt-6.1-sol(xhigh) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found; no P0 or P1 findings remain.
Reviewed changes — Reviewed faf4da0, the padding-contract fix since the previous Pullfrog review at ffc21af.
- Aligned padding types with Vega’s getter. Replaced representation branching with a documented local type adaptation, preserving annotation offsets while removing the required Doctor-gate failure.
Validation: the workspace build, typecheck, changed-file checks, and all 400 selected chart, metrics, and Doctor tests pass. The chart Doctor check passes with zero diagnostics, and five public Vega padding configurations confirm the getter contract.
gpt-6.1-sol(xhigh) ｜ 𝕏
Reports in Drop drawn with comark-ui read like a docs page, not like the dashboards we want to match, such as nuxt-evals and Slopalytics. Each chart sits in its own card with a bold caption, half the column's width, bars are fat with no numbers on them, lines are smoothed with a swatch legend, and the colors are Tailwind 500s. Three of the charts those pages lean on have no
::charttype at all: a matrix of counts or scores, a log axis, and money printed as$22.2k.This PR draws a chart as a dashboard section. It sits under a hairline with its title and a new
descriptionat the start, and spans the column: every chart over axes is drawn 744px wide, labels included, and scales to the column, so text matches from chart to chart. Upright bars stay about 40px wide (22px side by side), bars print their numbers past their ends by default (labels=falseturns that off; a stack prints its total past its end and each segment's number inside it where it fits), axes are 11px and quiet, lines are 1.5px with a point on every value, a line chart of two to eight series names each line where it ends, grid lines are dotted, and the palette is twelve muted OKLCH tones that each reach 3:1 on white and 6:1 on black. It also adds:type="heatmap": a cell per row and column, shaded by its number and printing it.better="higher"or"lower"colors the cells red, amber, green for scores.unit:$goes before the number,%and words after. From a thousand up, numbers print as three digits and a letter, on the bars and the axis.log,log="x",log="both": log scales that tick at 1, 2 and 5, with issues for zeros, stacks and pies.::stats{tiles}becomes one strip of figures with hairlines between them, value over name. The playground gains Heatmap, Scores, Money and Log scale presets.This is breaking. Charts lose their card and
data-comark-frame, scale up to the column instead of keeping their drawn size,--cch-paddingdefaults to 0, numbers print shortened and past their bars, and the series colors change. A pie, a heatmap and a chart with notes keep their own size. The theming docs show how to put the card back with CSS. The three new lint rules have Doctor pages, and thereportandcomark-uiskills now point at heatmaps,logandunit.Before/after, with the two references: https://drop.vitehub.dev/d/r9dmr8kxbj41/charts-span-the-column. Every chart type in the new style: https://drop.vitehub.dev/d/kpwa09ft1s22/chart-gallery-every-chart-type-in-the-report-style
The chart e2e corpus passes. It caught numbers past full-height bars clipping at the SVG's edge and 60-step dated waterfalls whose angled first name met the corner's
0, so labeled bars keep room past their ends and crowded waterfall names hang straight down. A run of the data-report skill on real data (#407) found that values were checked against each other with a margin that grew with each label (taken.some(covers)passed the index as the margin), so later bars lost their numbers; a test with 30 bars fails without the fix. Charts, metrics, lint and ui tests,vp check, both typechecks, Vite Doctor and the docs lint pass.remark-to-comark's "refuses to migrate with no git repository unless forced" hangs for about 300 s on Slimbook whether run alone or in the suite; that package imports nothing this PR touches. I haven't built the whole docs site, only its playground presets, or looked at the Vue, React and Svelte output in a browser; their markup is covered byframeworks.test.ts.