Repository navigation
fix(ErrorBar): clip error bars with their graphical item - #8054
PavelVanecek merged 1 commit into
Conversation
When an axis has allowDataOverflow, Line, Bar and Scatter clip their shapes to the plot area, but ErrorBar renders in its own zIndex layer and so it escaped that clip path. Pass the clip path of the graphical item through ErrorBarContext and set it on the error bars layer, as 2.x did since #3300. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughError bars now inherit the clip path of their parent Line, Bar, or Scatter when axis data overflow clipping is enabled. Tests cover clipping behavior for all three chart elements. ChangesErrorBar clipping
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description is complete and follows the repository template. It explains the bug, links issue #8053, describes the motivation, documents testing, identifies the change as a bug fix, and confirms a…Title check The title clearly and concisely describes the main change: clipping ErrorBar elements with their graphical item. Linked Issues check Issue #8053requires ErrorBar elements in Line, Bar, and Scatter to use the same clip path as the graphical item when allowDataOverflow is enabled. The PR adds clipPath to ErrorBarContext. Line, Bar, …Out of Scope Changes check The changes are limited to ErrorBarContext, ErrorBar, Line, Bar, Scatter, and focused ErrorBar tests. These changes implement or verify issue #8053. No unrelated behavior or files are identified.Full details: Description check
-
Explanation
The description is complete and follows the repository template. It explains the bug, links issue
#8053, describes the motivation, documents testing, identifies the change as a bug fix, and confirms added tests.
Full details: Linked Issues check
-
Explanation
Issue
#8053requires ErrorBar elements in Line, Bar, and Scatter to use the same clip path as the graphical item when allowDataOverflow is enabled. The PR adds clipPath to ErrorBarContext. Line, Bar, and Scatter pass their item clip path, or undefined when clipping is not required. ErrorBar applies the value to the recharts-errorBars layer. The added ErrorBar tests cover clipping for all three chart types and no clipping without allowDataOverflow.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Bundle ReportChanges will increase total bundle size by 1.06kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8054 +/- ##
=======================================
Coverage 89.14% 89.14%
=======================================
Files 653 653
Lines 16194 16196 +2
Branches 4299 4301 +2
=======================================
+ Hits 14436 14438 +2
Misses 1564 1564
Partials 194 194 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Test Results SummaryThis comment shows the latest test results for this pull request.
ℹ️ About Flakiness.io in GitHubYour team has set up Flakiness.io pull request comments. This comment is updated when
Flakiness.io rewrites the comment in place, so it always reflects the most recently tested commit. |
|
Ran this on your branch. ErrorBar spec: 46/46 pass, including the four new clipping tests. On main with only your test file applied, the three "should clip" tests fail and the "should not clip" case passes, so the tests pin the fix. Line, Bar and Scatter specs still pass (41 tests), and tsc is clean. One thing I checked while reading: the no-overflow case leaves clipPath undefined, so error bars outside an allowDataOverflow axis keep their current unclipped rendering - that matches the 2.x behavior you referenced. Nicely covered by the negative test. Small note for reviewers: the change threads the parent's clip path through ErrorBarContext, so custom content rendering an ErrorBar outside a Line/Bar/Scatter gets clipPath undefined and behaves exactly as before. Nothing to change, just confirming the fallback. |
Description
ErrorBarrenders in its ownZIndexLayer, which is portalled out of the graphical item's layer, so it does not inherit the clip path thatBarandScatterput on their layers when an axis hasallowDataOverflow.Lineonly clips its curve and dots, so its error bars were not clipped either.This adds a
clipPathtoErrorBarContext.Line,BarandScatterpass their own clip path, orundefinedwhen they are not clipped, andErrorBarsets it on therecharts-errorBarslayer. That is the same clip path 2.x applied to the error bars since #3300.Related Issue
Fixes #8053
Motivation and Context
With
allowDataOverflowand a domain smaller than the error range, the whiskers are drawn over the axes and the chart margin while the item itself is clipped. This was reported in #2978, fixed in 2.4.0, and came back in 3.x.How Has This Been Tested?
test/cartesian/ErrorBar.spec.tsxthat renderLine,BarandScatterwith<YAxis domain={[0, 100]} allowDataOverflow />and check that every error bar is inside an element clipped by the item's clip path, plus one that checks nothing is clipped withoutallowDataOverflow. The three clipping tests fail onmainand pass with this change.npx vitest run --project unit:lib,npm run test-omnidoc,npm run check-types-lib,npm run check-types-test, and eslint and prettier on the changed files.YAxis domain={[0, 100]} allowDataOverflowand compared them in Chrome before and after.Screenshots (if appropriate):
Before: the whiskers of the points above 100 and below 0 are drawn above the plot area and over the XAxis tick labels, in all three charts.
After: they are cut at the edges of the plot area, like the line, bars and dots.
Types of changes
Checklist:
I used Claude Code to write this change.
Summary by CodeRabbit