fix(charts): one gridline treatment for cartesian axes, keep category labels when compact (#1247) - #1271
Merged
Merged
Conversation
… labels when compact (#1247) Two defects made charts on one dashboard look like they came from different tools. Gridlines: bar and line already inherit the theme's splitLine, so the drift was narrower than reported — neither theme registers a `timeAxis` entry, so the gantt and any date-based line chart fell back to ECharts' un-themed light grey, which reads near-white on the dark canvas. Both themes now register timeAxis, the colour is the GRID_LINE_COLOR token, and the gantt's local `dashed / opacity 0.3` override is gone. Axis labels: buildCategoryAxisLabel returned `show: !compact`, so under 300px the bar widget dropped category identification entirely — four unlabelled bars. Compact now keeps the labels; the existing sub-400px rule already truncates them to 10 chars, which also leaves the `compact` option dead, so it goes. (A tighter 6-char budget was tried first and collapsed "Widget A".."Widget G" into seven identical stubs.) cartesian-axis-defaults.test.tsx pins the option shape across bar, line and gantt so a future chart re-adding its own splitLine.lineStyle, or hiding category labels at a breakpoint, fails loudly. Verified in Storybook, both themes: appshell, bar, gantt, and a 280px bar widget. Closes #1247 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alfredo1996
added
pkg:component
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCartesian charts preserve category labels in compact containers, centralize gridline styling in light and dark themes, apply shared styling to time axes, and add tests for emitted ECharts options across chart types. ChangesCartesian chart defaults
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🤖 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 `@component/src/charts/line-chart.tsx`:
- Around line 254-256: Update the compact category-axis configuration in the
line chart to use the shared buildCategoryAxisLabel helper with the xValues
length and container width, while retaining { show: true } for time axes. Add
width to the relevant memo dependency list so truncation responds to resizing,
and extend cartesian-axis-defaults.test.tsx to assert compact line-label
truncation.
🪄 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 Plus
Run ID: 840cd18a-21f4-4f20-bd77-5316d48cf8fb
📒 Files selected for processing (8)
.claude/skills/design-review/skill.mdcomponent/src/charts/__tests__/axis-label-utils.test.tscomponent/src/charts/__tests__/cartesian-axis-defaults.test.tsxcomponent/src/charts/bar-chart.tsxcomponent/src/charts/chart-utils.tscomponent/src/charts/gantt-chart.tsxcomponent/src/charts/line-chart.tsxcomponent/src/charts/theme.ts
💤 Files with no reviewable changes (1)
- component/src/charts/bar-chart.tsx
…label config CodeRabbit on #1271: the line chart kept labels visible in compact mode but never truncated them, so a narrow widget with long category names rendered overlapping text where the bar chart truncated cleanly — the cross-chart inconsistency #1247 exists to remove. Only the category branch takes buildCategoryAxisLabel. A time axis keeps plain labels: ECharts formats and thins dates itself, and the width-based rotation heuristic is meaningless for them. That distinction is what made this safe to apply — the original objection was that routing every x axis through the helper would add rotation to dense time series. Built as two object literals rather than one with ternaries, because ECharts discriminates the axis union on `type` and a "time" | "category" union defeats narrowing. `width` joins the memo deps so truncation responds to resize. Refs #1247 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Applied — good catch, and the distinction you drew is what made it safe. My original reason for not routing the line chart through Leaving it as it was would have perpetuated precisely the inconsistency #1247 exists to remove. Two things worth noting for review:
Test added to |
|
3 tasks
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
2 participants
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.
Closes #1247
What was actually wrong
The issue reported "bar draws heavy gridlines, line's are almost invisible". Reading the code, bar and line both already inherit the theme's
splitLine— the drift was elsewhere:timeAxiswas never registered in either theme. The themes definecategoryAxisandvalueAxisonly, so the gantt (type: "time") and any date-based line chart fell back to ECharts' un-themed light grey — near-white on the dark canvas. This is the real "different tools on one dashboard" cause.dashed / opacity 0.3override, the one genuine per-chart gridline style in the package.buildCategoryAxisLabelreturnedshow: !compact— under 300px the bar widget dropped category identification entirely. That is the four unlabelled bars inappshell-light.png.Changes
GRID_LINE_COLORtoken incharts/theme.ts, used by both themes;timeAxisregistered alongsidecategoryAxis/valueAxis.splitLine: { show }.compactoption dead, and it is removed. A tighter 6-char budget was tried first and collapsedWidget A..Widget Ginto seven identicalWidge…stubs.Tests
component/src/charts/__tests__/cartesian-axis-defaults.test.tsx(new) pins the option shape across bar, line and gantt:splitLine.lineStyleaxis-label-utils.test.tsgains a common-prefix case so an over-tight truncation budget fails loudly.Verification
npm run verify— 248 + 116 + 28 files, all greenAuthPage.login: failed to reach /(login contention, no chart assertion among them); 19 passed on re-run, the remaining 2 pass in isolation (14/14)Known, out of scope
The gantt's dataZoom slider renders in ECharts' default lavender-blue in both themes. Not filed.
🤖 Generated with Claude Code
Summary by CodeRabbit