Skip to content

fix(engine): editorial defaults audit#90

Merged
rileyhilliard merged 1 commit into
mainfrom
worktree-editorial-defaults-audit
Jul 5, 2026
Merged

fix(engine): editorial defaults audit#90
rileyhilliard merged 1 commit into
mainfrom
worktree-editorial-defaults-audit

Conversation

@rileyhilliard

@rileyhilliard rileyhilliard commented Jul 5, 2026

Copy link
Copy Markdown
Member

Summary

Audited all rendering defaults against the 12-item editorial design checklist (plan 04). 9 items passed, 3 fixed:

  • Tabular figures on all numeric text: added font-variant-numeric: tabular-nums to endpoint label values, bar/column/pie/dot data labels, and barlist value labels. Axis ticks, metrics, tooltips, and table cells already had it.
  • X domain line hidden for non-grounded marks: line, area, scatter, and dot charts no longer render the x-axis baseline. Bar/tick/lollipop keep it. User-explicit axis.domain always wins.
  • Scatter vertical gridlines: scatter charts now show x-axis gridlines by default (editorial convention for two-quantitative-axis charts). User axis.grid overrides.

Test plan

  • All 2467 tests pass
  • Lint, typecheck, build green
  • 8 visual baselines regenerated (line/area charts lost their x domain line)
  • 20/20 visual regression tests pass
  • Code review: moved GROUNDED_MARKS to module scope, added missing dot chart tabular-nums

https://claude.ai/code/session_01S8jmshMshTq7Gs2RYKsd5w

Summary by CodeRabbit

  • Bug Fixes
    • Improved x-axis defaults so baseline and gridlines now appear more consistently based on chart type.
    • Fixed axis baseline behavior in rendering tests to match grounded vs. non-grounded charts.
    • Made numeric labels align more evenly across bars, columns, dots, pies, and endpoint labels.
    • Updated axis property expectations so default settings are handled more accurately.

…atter gridlines

Audited all rendering defaults against editorial design standards (FT,
Economist, Datawrapper conventions). Three fixes:

1. Tabular figures on all numeric text: added font-variant-numeric:
   tabular-nums to endpoint label values, bar/column/pie/dot data
   labels, and barlist value labels. Axis ticks, metric values, tooltip
   values, and table cells already had it.

2. X domain line hidden for non-grounded marks: line, area, scatter,
   and dot charts no longer render the x-axis baseline since their marks
   float above it. Bar, tick, and lollipop keep the domain line where
   marks are visually grounded. User-explicit axis.domain always wins.

3. Scatter charts get vertical gridlines by default, matching editorial
   convention for two-quantitative-axis charts.

Claude-Session: https://claude.ai/code/session_01S8jmshMshTq7Gs2RYKsd5w
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3ebe5b51-9733-4348-a23d-edafc60b15e1

📥 Commits

Reviewing files that changed from the base of the PR and between d23251d and 3d47038.

⛔ Files ignored due to path filters (9)
  • e2e/visual/__screenshots__/stories.spec.ts-snapshots/chart-with-watermark-visual-darwin.png is excluded by !**/*.png
  • e2e/visual/__screenshots__/stories.spec.ts-snapshots/compact-line-320px-visual-darwin.png is excluded by !**/*.png
  • e2e/visual/__screenshots__/stories.spec.ts-snapshots/dark-mode-line-visual-darwin.png is excluded by !**/*.png
  • e2e/visual/__screenshots__/stories.spec.ts-snapshots/legend-bottom-right-visual-darwin.png is excluded by !**/*.png
  • e2e/visual/__screenshots__/stories.spec.ts-snapshots/legend-top-visual-darwin.png is excluded by !**/*.png
  • e2e/visual/__screenshots__/stories.spec.ts-snapshots/line-multi-series-visual-darwin.png is excluded by !**/*.png
  • e2e/visual/__screenshots__/stories.spec.ts-snapshots/multi-series-area-overlap-visual-darwin.png is excluded by !**/*.png
  • e2e/visual/__screenshots__/stories.spec.ts-snapshots/multi-series-area-stacked-visual-darwin.png is excluded by !**/*.png
  • packages/engine/src/__tests__/__snapshots__/compile-snapshot.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (9)
  • packages/engine/src/__tests__/axes.test.ts
  • packages/engine/src/barlist/compile-barlist.ts
  • packages/engine/src/charts/bar/labels.ts
  • packages/engine/src/charts/column/labels.ts
  • packages/engine/src/charts/dot/labels.ts
  • packages/engine/src/charts/pie/labels.ts
  • packages/engine/src/endpoint-labels/compute.ts
  • packages/engine/src/layout/axes.ts
  • packages/vanilla/src/__tests__/svg-renderer.test.ts

📝 Walkthrough

Walkthrough

This PR adds fontVariant: 'tabular-nums' to numeric label styles across bar list, bar, column, dot, pie, and endpoint-label renderers for consistent glyph alignment. It also updates x-axis default logic to conditionally set domainLine and gridlines based on markType, introducing a GROUNDED_MARKS set, with corresponding test updates.

Changes

Tabular Numerals and Mark-Aware Axis Defaults

Layer / File(s) Summary
Tabular-nums font variant for numeric labels
packages/engine/src/barlist/compile-barlist.ts, packages/engine/src/charts/bar/labels.ts, packages/engine/src/charts/column/labels.ts, packages/engine/src/charts/dot/labels.ts, packages/engine/src/charts/pie/labels.ts, packages/engine/src/endpoint-labels/compute.ts
Adds fontVariant: 'tabular-nums' to numeric label styles across bar list, bar, column, dot, pie, and endpoint-label value rendering.
Mark-aware x-axis domainLine/gridline defaults
packages/engine/src/layout/axes.ts, packages/engine/src/__tests__/axes.test.ts, packages/vanilla/src/__tests__/svg-renderer.test.ts
Introduces GROUNDED_MARKS (bar, tick, lollipop) to compute x-axis domainLine defaults, changes gridlines default to enable for point marks, and updates unit/integration tests to reflect grounded vs. non-grounded baseline behavior.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

  • tryopendata/openchart#64: Both PRs modify the same computeAxes logic in packages/engine/src/layout/axes.ts affecting x/y axis defaults and gridline/tick handling, with related test updates.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the PR’s main theme: auditing and fixing editorial defaults in the engine.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-editorial-defaults-audit

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@rileyhilliard rileyhilliard merged commit 769e927 into main Jul 5, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant