fix: tighten chrome-to-legend gap and y-axis title clearance#77
Conversation
Two layout spacing fixes: 1. Top legend gap: when a top legend is present, legendGap already separates it from what's above, so adding the full topAxisGap (axisMargin + inlineTickOverhang) created redundant spacing. Now only adds inlineTickOverhang when a top legend exists. 2. Y-axis title cramping: AXIS_TITLE_GAP (distance from tick label edge to title center) was 8px, but the rotated glyph extends ~7px from center, leaving only 1px visual clearance. Bumped to 14px for ~7px breathing room. Also fixed renderer using hardcoded AXIS_TITLE_OFFSET_COMPACT floor instead of width-aware getAxisTitleOffset() that dimensions.ts uses.
|
Warning Review limit reached
More reviews will be available in 35 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR refines chart layout spacing by increasing the y-axis title gap constant from 8 to 14 pixels across the dimensions engine and renderer, and fixes top legend margin calculation to prevent double-counting spacing by using ChangesChart Dimension and Axis Layout Spacing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/engine/src/layout/dimensions.ts (1)
638-640: ⚡ Quick winConsider sharing
AXIS_TITLE_GAPwith the renderer.This
AXIS_TITLE_GAP = 14is now duplicated as a local literal here and inpackages/vanilla/src/renderers/axes.ts(Line 275). The reservation here only matches the renderer's placement as long as both literals stay in sync; a future edit to one will silently reintroduce the renderer/dimensions mismatch this PR set out to fix. Exporting a single constant from@opendata-ai/openchart-core(alongsideTICK_LABEL_OFFSET/getAxisTitleOffset) would keep them locked together.🤖 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 `@packages/engine/src/layout/dimensions.ts` around lines 638 - 640, The literal AXIS_TITLE_GAP = 14 is duplicated and should be centralized; instead of hardcoding it in packages/engine/src/layout/dimensions.ts, export a single AXIS_TITLE_GAP constant from the core module alongside TICK_LABEL_OFFSET and getAxisTitleOffset, then import and use that exported AXIS_TITLE_GAP in dimensions.ts (where dynamicTitleOffset is computed) and in packages/vanilla/src/renderers/axes.ts so both renderer and layout share the same authoritative value.
🤖 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.
Inline comments:
In `@packages/engine/src/__tests__/dimensions.test.ts`:
- Around line 487-488: The test's threshold is too loose; update the assertion
for topMarginDelta (comparing dimsWithTopLegend and dimsNoLegend) to use the
correct smaller allowance by replacing the current bound
(topLegend.bounds.height + legendGap(600) + 1) with topLegend.bounds.height +
legendGap(600) + inlineTickOverhang (i.e., the inline tick overhang, not the
full topAxisGap) so the correct delta (~30) passes but the doubled bug (~36)
fails; look for dimsWithTopLegend, dimsNoLegend, topLegend, legendGap, and
inlineTickOverhang and the related logic modified around dimensions.ts (near the
change at line 679) to make this assertion precise.
---
Nitpick comments:
In `@packages/engine/src/layout/dimensions.ts`:
- Around line 638-640: The literal AXIS_TITLE_GAP = 14 is duplicated and should
be centralized; instead of hardcoding it in
packages/engine/src/layout/dimensions.ts, export a single AXIS_TITLE_GAP
constant from the core module alongside TICK_LABEL_OFFSET and
getAxisTitleOffset, then import and use that exported AXIS_TITLE_GAP in
dimensions.ts (where dynamicTitleOffset is computed) and in
packages/vanilla/src/renderers/axes.ts so both renderer and layout share the
same authoritative value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b4b1981-3f14-419a-bbdb-240ceb62499a
📒 Files selected for processing (3)
packages/engine/src/__tests__/dimensions.test.tspackages/engine/src/layout/dimensions.tspackages/vanilla/src/renderers/axes.ts
| const topMarginDelta = dimsWithTopLegend.margins.top - dimsNoLegend.margins.top; | ||
| expect(topMarginDelta).toBeLessThan(topLegend.bounds.height + legendGap(600) + 1); |
There was a problem hiding this comment.
This regression assertion is too loose to catch the bug it guards.
The comment shows the correct delta is 30 and the doubling-bug delta is 36, but the threshold is topLegend.bounds.height + legendGap(600) + 1 = 28 + 8 + 1 = 37. Both 30 < 37 and 36 < 37 pass, so reverting the fix (adding the full topAxisGap instead of inlineTickOverhang at dimensions.ts Line 679) would not fail this test. Tighten the bound so the doubling case (36) is excluded.
💚 Proposed tighter bound
- const topMarginDelta = dimsWithTopLegend.margins.top - dimsNoLegend.margins.top;
- expect(topMarginDelta).toBeLessThan(topLegend.bounds.height + legendGap(600) + 1);
+ const topMarginDelta = dimsWithTopLegend.margins.top - dimsNoLegend.margins.top;
+ // Doubling would add the full topAxisGap (= axisMargin + inlineTickOverhang)
+ // on top of legendHeight + legendGap; the correct path adds only
+ // inlineTickOverhang, so the delta must stay strictly below height + gap.
+ expect(topMarginDelta).toBeLessThan(topLegend.bounds.height + legendGap(600));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const topMarginDelta = dimsWithTopLegend.margins.top - dimsNoLegend.margins.top; | |
| expect(topMarginDelta).toBeLessThan(topLegend.bounds.height + legendGap(600) + 1); | |
| const topMarginDelta = dimsWithTopLegend.margins.top - dimsNoLegend.margins.top; | |
| // Doubling would add the full topAxisGap (= axisMargin + inlineTickOverhang) | |
| // on top of legendHeight + legendGap; the correct path adds only | |
| // inlineTickOverhang, so the delta must stay strictly below height + gap. | |
| expect(topMarginDelta).toBeLessThan(topLegend.bounds.height + legendGap(600)); |
🤖 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 `@packages/engine/src/__tests__/dimensions.test.ts` around lines 487 - 488, The
test's threshold is too loose; update the assertion for topMarginDelta
(comparing dimsWithTopLegend and dimsNoLegend) to use the correct smaller
allowance by replacing the current bound (topLegend.bounds.height +
legendGap(600) + 1) with topLegend.bounds.height + legendGap(600) +
inlineTickOverhang (i.e., the inline tick overhang, not the full topAxisGap) so
the correct delta (~30) passes but the doubled bug (~36) fails; look for
dimsWithTopLegend, dimsNoLegend, topLegend, legendGap, and inlineTickOverhang
and the related logic modified around dimensions.ts (near the change at line
679) to make this assertion precise.
Move AXIS_TITLE_GAP from duplicated local consts in dimensions.ts and axes.ts to a single export in core/responsive/metrics.ts. Fixes the same desync pattern (renderer vs engine using different values) that the prior commit corrected for AXIS_TITLE_OFFSET_COMPACT. Also fixes the stale comment in dimensions.ts that still referenced the old 8px gap and AXIS_TITLE_OFFSET_COMPACT.
1 participant
Summary
Test plan
Summary by CodeRabbit
Bug Fixes
Improvements