refactor(charts): centralize typography, dashes, strokes; flip y-axis to right#4722
Open
aseckin wants to merge 9 commits into
Open
refactor(charts): centralize typography, dashes, strokes; flip y-axis to right#4722aseckin wants to merge 9 commits into
aseckin wants to merge 9 commits into
Conversation
… to right Introduce three token modules under front_end/src/constants/: - chart_typography (font family, sizes, pre-composed styles with tabular-nums) - chart_dash (named dash patterns: grid, cursor, quartile, threshold, timeline) - chart_stroke (stroke widths and point sizes for resolution markers) - chart_tokens (ChartThemeOverride type + resolveChartTokens helper) Fix malformed font-family in chart_theme.ts (was using `var(--font-inter-variable) var(--font-inter)` without commas, silently falling back to system font). Raise base theme fontSize to 10 (was dead code at 9) and add fontVariantNumeric: tabular-nums. Per-chart axis styles now spread CHART_FONT_STYLE.tick / .axisLabel so tabular-nums and font family land on the rendered SVG (Victory's per-axis style replaces theme tickLabels). Apply CHART_DASH tokens at every strokeDasharray site across chart components and primitives, unifying previously divergent values for cursor/grid/quartile/threshold lines. Flip y-axis orientation to "right" everywhere (fan_chart, numeric_chart, multiple_choice_chart, group_chart, continuous_area_chart for discrete/CDF). Drop the offsetX = chartWidth + 5 hack and the VictoryLabel x=chartWidth axisLabelComponent overrides. FanChart default variant padding swapped from left-heavy to right-heavy. Discrete y-axis in ContinuousAreaChart now positions at xDomain[1]. Unify FanChart resolution diamond stroke width (2.5) and point size (10) across default and index variants. Extend STYLE_PROPERTIES in svg_export.ts to preserve font-variant-numeric and a handful of other CSS properties that were being dropped during SVG download. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
React has no `onMouseLeaveCapture` (mouseleave doesn't bubble, so no capture phase exists). React 19 logs an "Unknown event handler property" warning and silently drops the prop, leaving the cursor stuck on its last position when the mouse leaves the plot area. Switch to `onMouseOutCapture`, matching the equivalent pattern in `multiple_choice_chart.tsx:311`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FanChart default variant's xAxisStyle was missing a `tickLabels` block, so x-axis tick labels inherited the theme's default `fill: black` instead of the gray-600 used by the y-axis. Mismatch was visible on question detail pages — x-axis darker than y-axis. ChartCursorLabel (the "Today" / hover-date label rendered above the cursor in numeric, group, and multi-choice charts) wasn't applying CHART_FONT_STYLE either, so it rendered in the browser's default sans-serif and in pure black/white instead of Inter + gray-700. Both now spread CHART_FONT_STYLE.tick / .cursor for fontFamily, fontSize, and fontVariantNumeric, and use gray-600/700 fills to match adjacent axis labels. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "Today" indicator on Date-type continuous charts is a standalone VictoryLabel (separate from the cursor-label path) and was setting fontSize inline without a fontFamily, falling back to the browser's default serif. Spread CHART_FONT_STYLE.tooltip so it renders in Inter at the same 12px we use for other tooltip-tier labels. Adjust CHART_DASH grid from "3 2" to "2 3" and cursor from "5 2" to "2 2" based on visual review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nts; fix tooltip class typo Final consistency pass — pulls a few stragglers under the same token umbrella we introduced in the chart-consistency refactor. - Extend CHART_STROKE_WIDTH with userPoint (1.5), predictionRange (2), resolutionLine (2), timelineMarker (2), and fanCommunityLine (6). Values preserved exactly; just centralized so they're greppable and tunable from one place. - Replace hardcoded strokeWidth literals across primitives and chart bodies (resolution_diamond, prediction_with_range, line_cursor_points, group_timeline_markers_overlay, fan_chart, group_chart, continuous_area_chart) with the matching CHART_STROKE_WIDTH key. - chart_value_box.tsx: the RESOLVED label and value chip text were rendering in browser-default sans-serif (no fontFamily) and without tabular-nums on the numeric value. Now set fontFamily explicitly to CHART_FONT_FAMILY and apply font-variant-numeric: tabular-nums on the value text so digits align with adjacent axis ticks. CHIP_FONT_SIZE now derives from CHART_FONT_SIZE.tooltip instead of a hardcoded 12. - question_prediction_tooltip.tsx: pre-existing typo where the className concatenated as "text-xsfont-medium" (no space) so neither class applied. Split into "text-xs font-medium". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/components/charts/group_chart.tsx (1)
379-384:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
onMouseLeaveCapture(or guardrelatedTarget) to avoid false cursor resets.Line 379 switches to
onMouseOutCapture, which also fires when moving between child SVG nodes; this can deactivate the cursor while still inside the plot area and cause flicker/reset behavior.Suggested fix
- onMouseOutCapture: () => { + onMouseLeaveCapture: () => { if (!onCursorChange) return; inPlotRef.current = false; setIsCursorActive(false); setLocalCursorTimestamp(null); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front_end/src/components/charts/group_chart.tsx` around lines 379 - 384, The handler currently uses onMouseOutCapture which fires when moving between child SVG nodes and can falsely reset the cursor; in the event handler tied to onMouseOutCapture, change it to onMouseLeaveCapture (or add a guard checking event.relatedTarget to ensure the pointer actually left the plot area) so you only run the deactivation logic; update the element that registers onMouseOutCapture to use onMouseLeaveCapture (or wrap the existing handler with a relatedTarget check) and keep the existing side-effects (setting inPlotRef.current = false, setIsCursorActive(false), setLocalCursorTimestamp(null)) only when the pointer truly leaves and when onCursorChange is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@front_end/src/components/charts/group_chart.tsx`:
- Around line 379-384: The handler currently uses onMouseOutCapture which fires
when moving between child SVG nodes and can falsely reset the cursor; in the
event handler tied to onMouseOutCapture, change it to onMouseLeaveCapture (or
add a guard checking event.relatedTarget to ensure the pointer actually left the
plot area) so you only run the deactivation logic; update the element that
registers onMouseOutCapture to use onMouseLeaveCapture (or wrap the existing
handler with a relatedTarget check) and keep the existing side-effects (setting
inPlotRef.current = false, setIsCursorActive(false),
setLocalCursorTimestamp(null)) only when the pointer truly leaves and when
onCursorChange is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a3d3daa-f61e-48f1-a0cd-61eae551a5ca
📒 Files selected for processing (13)
front_end/src/components/charts/continuous_area_chart.tsxfront_end/src/components/charts/fan_chart.tsxfront_end/src/components/charts/fan_chart_variants.tsfront_end/src/components/charts/group_chart.tsxfront_end/src/components/charts/primitives/chart_cursor_label.tsxfront_end/src/components/charts/primitives/chart_value_box.tsxfront_end/src/components/charts/primitives/line_cursor_points.tsxfront_end/src/components/charts/primitives/prediction_with_range.tsxfront_end/src/components/charts/primitives/question_prediction_tooltip.tsxfront_end/src/components/charts/primitives/resolution_diamond.tsxfront_end/src/components/charts/primitives/timeline_markers/group_timeline_markers_overlay.tsxfront_end/src/constants/chart_dash.tsfront_end/src/constants/chart_stroke.ts
✅ Files skipped from review due to trivial changes (2)
- front_end/src/constants/chart_dash.ts
- front_end/src/components/charts/primitives/prediction_with_range.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- front_end/src/components/charts/primitives/resolution_diamond.tsx
- front_end/src/components/charts/primitives/timeline_markers/group_timeline_markers_overlay.tsx
- front_end/src/components/charts/fan_chart.tsx
- front_end/src/components/charts/fan_chart_variants.ts
- front_end/src/components/charts/continuous_area_chart.tsx
…cursor reset Code-review follow-up. In commit bb1d19a we replaced the typo'd `onMouseLeaveCapture` (which React 19 doesn't recognize — no capture phase exists for non-bubbling mouseleave) with `onMouseOutCapture`. The reviewer correctly noted that `mouseout` fires on every child-node transition inside the SVG, which can falsely toggle `inPlotRef.current = false` in the brief window before the next `mousemove` re-evaluates the plot bounds. That gates `onCursorChange` (line ~320), so cursor updates can silently drop. Switch to plain `onMouseLeave` — fires once when the pointer truly leaves the element, no capture phase needed since the event doesn't bubble. This mirrors the pattern already used in numeric_chart.tsx:311. Victory's events array passes the handler straight to the underlying React element, so no Victory-specific wiring is required. The reviewer suggested `onMouseLeaveCapture` or a relatedTarget guard; `onMouseLeaveCapture` is the same prop name React 19 rejected, and the guard is unnecessary once we use the right event. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce three token modules under front_end/src/constants/:
Fix malformed font-family in chart_theme.ts (was using
var(--font-inter-variable) var(--font-inter)without commas, silently falling back to system font). Raise base theme fontSize to 10 (was dead code at 9) and add fontVariantNumeric: tabular-nums.Per-chart axis styles now spread CHART_FONT_STYLE.tick / .axisLabel so tabular-nums and font family land on the rendered SVG (Victory's per-axis style replaces theme tickLabels). Apply CHART_DASH tokens at every strokeDasharray site across chart components and primitives, unifying previously divergent values for cursor/grid/quartile/threshold lines.
Flip y-axis orientation to "right" everywhere (fan_chart, numeric_chart, multiple_choice_chart, group_chart, continuous_area_chart for discrete/CDF). Drop the offsetX = chartWidth + 5 hack and the VictoryLabel x=chartWidth axisLabelComponent overrides. FanChart default variant padding swapped from left-heavy to right-heavy. Discrete y-axis in ContinuousAreaChart now positions at xDomain[1].
Unify FanChart resolution diamond stroke width (2.5) and point size (10) across default and index variants.
Extend STYLE_PROPERTIES in svg_export.ts to preserve font-variant-numeric and a handful of other CSS properties that were being dropped during SVG download.
Summary by CodeRabbit
Bug Fixes
Improvements