feat(quill-charts): canvas tick marks and axis-line coexistence - #67990
Conversation
|
Reviews (1): Last reviewed commit: "feat(quill-charts): canvas tick marks an..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 sp-ship · 🚫 Blockers
The feature itself is clean: tick marks draw on the static layer (no hover-path cost), pixel-snapping and axis-line colour are centralized, and tick geometry reuses the same visible-label helpers as AxisLabels so ticks can't drift. Two things block merge — IMPROVEMENTS.md, an internal working-notes file whose own text says to keep it untracked and out of the public repo, is committed here; and the new resolveAxisLineColor precedence helper (the shared source of the tick/axis-line colour) ships with no test. Non-blocking: a horizontal-orientation grid/axis-line double-line edge case, a now-stale showAxisLines doc contract, and duplication between the tickMarkCoords memo and AxisLabels.
sampennington
added
the
stamphog
PostHog
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both greptile blockers (accidental internal-notes file commit, missing precedence test) were fixed on the current head, remaining review comments are minor deferred cosmetic follow-ups (stale JSDoc, an edge-case double gridline in a rare horizontal config) that the reviewer acknowledged with 👍, and there's no evidence of unresolved production-risk issues in this canvas-rendering-only chart library change.
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
All substantive blockers raised by the independent bot reviewer (internal notes file committed to the public repo, untested color-precedence helper, inconsistent pixel-snapping) were fixed in later commits and the current diff confirms the fixes; remaining items (stale JSDoc, a narrow horizontal-orientation edge case) are cosmetic/visual-only and were explicitly logged as follow-ups rather than left silently unaddressed.
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
Opt-in, default-off axis restyle capabilities: - showTickMarks: axis tick marks drawn on canvas in the same pass and with the same half-pixel snapping (snapToPixel) as drawAxes/drawGrid, so ticks, axis, grid, and zero baseline land on identical pixels — a DOM overlay can't guarantee that across subpixel rounding - theme.axisLineColor via resolveAxisLineColor(theme): mute the L-axis and tick marks without muting tick-label text; one resolver shared by every call site so the precedence can't drift - with showAxisLines: the grid skips its plot-edge frame (no stray far border) and category lines, the bottom-hugging gridline yields to the chart-drawn axis (framed grids keep theirs), bar/combo charts draw the axis L alongside the grid like line charts, and axes render after the data so nothing paints over the baseline; grid frame closing strokes share the same snapped pixel grid Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
78ef097 to
066082f
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile flagged a concrete rendering bug — the new frameless-grid path skips the zero-tick filter in the horizontal branch of drawGrid, so horizontal bar/combo charts with showAxisLines can double-draw a gridline against the axis — and the author's own reply agrees it's valid but explicitly defers the fix to a follow-up rather than fixing it here; the diff confirms the horizontal value-tick loop still has no !frame guard while the vertical one does. That's a real, reachable defect in the exact feature this PR ships, left unaddressed. Additionally, all reviews (greptile, graphite, and the author's own review-persona comments) are on older commits, not the current head, so there's no independent sign-off on what's actually being merged.
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
25.39 MiB | 🔺 +1.3 KiB (+0.0%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
3.76 MiB · 695 files | no change | █████████░ 87.6% of 4.29 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
11.52 MiB · 2,937 files | no change | █████████░ 89.5% of 12.87 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 275.2 KiB | ../node_modules/.pnpm/posthog-js@1.396.5/node_modules/posthog-js/dist/rrweb.js |
| 266.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.37.4_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 224.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 211.3 KiB | ../node_modules/.pnpm/posthog-js@1.396.5/node_modules/posthog-js/dist/module.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.9 KiB | src/lib/api.ts |
| 69.7 KiB | src/products.tsx |
| 61.7 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 57.2 KiB | src/lib/utils/eventUsageLogic.ts |
| 39.6 KiB | src/lib/KeaDevTools.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 458.3 KiB | ../node_modules/.pnpm/@posthog+brand@0.3.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/driving-hogzilla.mjs |
| 302.9 KiB | ../node_modules/.pnpm/@posthog+brand@0.3.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/coffee-run.mjs |
| 275.2 KiB | ../node_modules/.pnpm/posthog-js@1.396.5/node_modules/posthog-js/dist/rrweb.js |
| 266.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.37.4_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 253.2 KiB | ../node_modules/.pnpm/@posthog+brand@0.3.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/chart-hog.mjs |
| 239.2 KiB | ../node_modules/.pnpm/@posthog+brand@0.3.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/x-ray.mjs |
| 224.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 211.6 KiB | ../node_modules/.pnpm/@posthog+brand@0.3.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/money.mjs |
| 211.3 KiB | ../node_modules/.pnpm/posthog-js@1.396.5/node_modules/posthog-js/dist/module.js |
| 185.9 KiB | ../node_modules/.pnpm/@posthog+brand@0.3.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/hogpatch.mjs |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Dist folder size — 🟢 -285.36 MiB (-16.3%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1464.68 MiB · 🟢 -285.36 MiB (-16.3%)
The frameless-grid skip of axis-hugging value gridlines only covered the vertical orientation; horizontal charts with showAxisLines could double-draw the zero gridline against the chart-drawn value axis. Mirror the guard in the horizontal branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Opt-in canvas chart feature (tick marks + axis-line/grid coexistence) with no default-behavior change; the diff confirms greptile's blocking findings were actually fixed (accidental IMPROVEMENTS.md removed, resolveAxisLineColor now unit-tested, and the horizontal frameless-baseline guard is present in canvas-renderer.ts despite the thread saying it was deferred), and remaining items are cosmetic doc follow-ups. Greptile and a security bot both 👍'd the PR, satisfying independent review for this complexity tier.
1 participant
Problem
Part 2 of the quill charts style refresh (stacked on #67989, split from #67982). Charts that draw the L-shaped axis lines can't show tick marks that stay pixel-aligned with the axis (a DOM-overlay attempt needed a 1px overlap fudge and still drifted), the axis color is tied to the tick-label color, and the grid's boxed frame fights the axis-line style — a stray right border and doubled baselines.
Changes
Library-only, opt-in, default-off.
showTickMarks— axis tick marks drawn on canvas in the same pass and with the same half-pixel snapping (snapToPixel) asdrawAxes/drawGrid, so ticks, axis, grid, and zero baseline land on identical pixels. Tick positions mirrorAxisLabels' visible-label computation so every tick pairs with a rendered label (multi-axis gutters and horizontal bars included).theme.axisLineColorvia a singleresolveAxisLineColor(theme)— mute the axis L and ticks without muting label text. One resolver shared by every call site (a copy-pasted fallback chain caused a stale-memo bug in the earlier draft; centralizing kills the class).showAxisLines: the grid skips its plot-edge frame and category lines, the bottom-hugging gridline yields to the chart-drawn axis (framed/classic grids keep their baseline gridline — the earlier unconditional skip was flagged in review as a default-rendering regression and is now gated), bar/combo charts draw the axis alongside the grid like line charts, and axes render after the data.How did you test this code?
Automatic notifications
Docs update
packages/quill/packages/charts/AGENTS.mdupdated with the new options in this PR.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code with the human directing the visuals against the live app. Part of the four-PR stack replacing the monolithic #67982; folds in the sp-ship review findings (gated bottom-gridline skip, resolveAxisLineColor extraction fixing the missing memo dep, spurious createScales dep dropped, snapToPixel adopted across drawGrid).
🤖 Generated with Claude Code