feat(quill-charts): dash patterns for grid lines and the hover crosshair - #67994
Conversation
|
Reviews (1): Last reviewed commit: "feat(quill-charts): dash patterns for gr..." | Re-trigger Greptile |
sampennington
left a comment
There was a problem hiding this comment.
🤖 sp-ship · 🚫 Blockers
Clean, additive, opt-in change — reviewers verified the setLineDash reset is leak-safe on every path (interior dash never bleeds onto the solid plot-edge frame or the sibling PR's axis-line strokes), the new ChartTheme fields are exported via index.ts, and the theme.crosshairDashPattern memo dep is referentially stable. The one blocker is coverage: the crosshair-dash half of the PR ships with no test, so its wiring could regress to green. Grid-dash coverage is also vertical-only. Minor naming/signature nits noted inline.
There was a problem hiding this comment.
Small, additive, opt-in feature (new optional dash-pattern theme fields for grid lines and crosshair) with resets to solid on every draw path; both test-coverage gaps flagged by the automated reviewer were fixed in follow-up commits and tests now cover vertical/horizontal grid and crosshair dash cases. Remaining open comments are non-blocking naming/API-shape suggestions explicitly left as author's call.
9291598 to
e4d02de
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
|
👋 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. |
There was a problem hiding this comment.
Greptile's independent review flagged missing test coverage for the crosshair dash reset and for the horizontal grid orientation; the thread claims these were fixed in later commits and marks them resolved, but the actual current diff only contains the single vertical-only grid-dash test with the same "narrating comment" reviewers said was removed — no drawCrosshair/composeDrawHoverWithCrosshair test exists anywhere in the test file.
78ef097 to
066082f
Compare
e4d02de to
17ceee4
Compare
🤖 CI report
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
3.76 MiB · 695 files | 🟢 -6 B (-0.0%) | █████████░ 87.6% of 4.29 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
11.52 MiB · 2,937 files | 🟢 -6 B (-0.0%) | █████████░ 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 — 🟢 -298.58 MiB (-16.9%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1464.68 MiB · 🟢 -298.58 MiB (-16.9%)
17ceee4 to
8f2842f
Compare
theme.gridDashPattern and theme.crosshairDashPattern (e.g. [3, 3]) dash the interior grid lines and the hover crosshair. Solid when omitted; the grid's plot-edge frame strokes stay solid either way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8f2842f to
54dbdfa
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Small, additive, opt-in charting feature (dash patterns for grid lines and crosshair) with consistent call-site wiring, proper line-dash resets to avoid bleed onto solid frame strokes, doc update in AGENTS.md, and tests added covering both orientations plus the crosshair path — verified in the diff that the previously-flagged gaps (horizontal grid branch, drawCrosshair dash) were actually addressed, not just claimed. Greptile (independent bot reviewer) reviewed, its concerns were resolved, and it reacted 👍.
Problem
Part 3 of the quill charts style refresh (stacked on #67990). The grid and hover crosshair can only draw solid lines; the refreshed style wants both as a subtle dashed texture.
Changes
Two theme slots, default-off:
theme.gridDashPattern— dashes the interior grid lines; the plot-edge frame strokes stay solid (matching how recharts keeps its axis line solid under a dashed grid)theme.crosshairDashPattern— dashes the hover crosshairHow did you test this code?
[3, 3]on grid and crosshair in the local appAutomatic notifications
Docs update
AGENTS.md restyle blurb extended with the two slots in this PR.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code. Part of the four-PR stack replacing the monolithic #67982.
🤖 Generated with Claude Code