Conversation
WalkthroughAdds an optional shape prop to Line (ActiveShape<CurveProps, SVGPathElement>), switches Line to render curves via the Shape wrapper, extends Shape utilities to include 'curve', adds a Storybook example demonstrating a custom line shape with ticks, and documents the new Line Changes
Sequence Diagram(s)sequenceDiagram
participant App as Line component
participant ShapeUtil as Shape selector
participant CurveComp as Curve
participant Custom as Custom shape renderer
App->>App: receive props (curveProps, optional shape)
alt shape provided (custom)
App->>ShapeUtil: render(shapeType:"curve", curveProps, option=shape)
ShapeUtil->>Custom: call custom renderer with curveProps & pathRef
Custom->>CurveComp: include Curve (base path) using curveProps
Custom-->>App: return combined SVG (path + ticks)
else no custom shape
App->>ShapeUtil: render(shapeType:"curve", curveProps)
ShapeUtil->>CurveComp: render default Curve with curveProps
CurveComp-->>App: return SVG path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)storybook/**📄 CodeRabbit inference engine (DEVELOPING.md)
Files:
storybook/stories/**/*.stories.tsx📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🧬 Code graph analysis (1)storybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsx (3)
🔇 Additional comments (2)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/util/ActiveShapeUtils.tsx (1)
65-67: Case 'curve' rendering via ShapeSelector LGTM; clarify boolean semantics.This mirrors existing shapes. One ask: document that passing shape={false} (option is boolean) falls back to default Curve rendering rather than disabling it, to avoid confusion.
src/cartesian/Line.tsx (1)
310-310: Rendering via Shape preserves default behavior; add guidance on pathRef.The delegation to Shape with shapeType="curve" and option={shape} is correct. Animation length relies on pathRef; custom shape functions/elements must forward props.pathRef to their main for animations to work.
- Action: add a short note to Line's shape prop docs: “If you provide a custom renderer, pass props.pathRef to your primary to retain length-based animations.”
Also applies to: 325-333
storybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsx (2)
81-86: Minor: ensure color fallback for ticks.If
strokeis undefined,currentColorwill be unset. Consider defaulting to a sensible value.- return ( - <g style={{ color: restProps.stroke }}> + const color = restProps.stroke ?? '#8884d8'; + return ( + <g style={{ color }}>
88-115: Add a minimal Storybook play test to smoke‑check custom shape rendering.Consider a play function that asserts the curve path and at least one tick element are present. Helps guard against regressions without heavy VR usage.
// after export const CustomLineShapeChart = { ... } CustomLineShapeChart.play = async ({ canvasElement }) => { const canvas = within(canvasElement); await expect(canvas.findByRole('img', { name: /legend/i })).resolves.toBeTruthy(); const paths = canvas.getAllByTestId(/recharts-curve|path/i); expect(paths.length).toBeGreaterThan(0); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cartesian/Line.tsx(7 hunks)src/util/ActiveShapeUtils.tsx(3 hunks)storybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Ensure code lints cleanly before submitting PRs (npm run lint)
Never use the TypeScript any type (implicit or explicit)
Prefer unknown over any and refine types appropriately
Explicitly type all function parameters and return values; do not rely on implicit any or inference
Do not use as type assertions; the only exception is as const
Files:
src/cartesian/Line.tsxstorybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsxsrc/util/ActiveShapeUtils.tsx
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any user-facing strings or formatting choices in the library code; leave internationalization to library users
Files:
src/cartesian/Line.tsxsrc/util/ActiveShapeUtils.tsx
storybook/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Use Storybook for high-fidelity examples intended for publication; prefer unit or VR tests for low-fidelity cases due to Chromatic limits
Files:
storybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsx
storybook/stories/**/*.stories.tsx
📄 CodeRabbit inference engine (CONTRIBUTING.md)
storybook/stories/**/*.stories.tsx: Use Storybook stories as smoke tests and add play-function tests with assertions when appropriate
If APIs change, ensure all Storybook stories still work as expected
Files:
storybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsx
🧬 Code graph analysis (3)
src/cartesian/Line.tsx (2)
src/util/types.ts (1)
ActiveShape(1037-1042)src/util/ActiveShapeUtils.tsx (1)
Shape(80-108)
storybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsx (2)
storybook/stories/API/props/ChartProps.ts (1)
CategoricalChartProps(72-230)src/util/isWellBehavedNumber.ts (1)
isWellBehavedNumber(1-3)
src/util/ActiveShapeUtils.tsx (2)
src/index.ts (1)
Curve(34-34)src/shape/Curve.tsx (1)
Curve(182-200)
🔇 Additional comments (4)
src/util/ActiveShapeUtils.tsx (2)
10-10: Curve import wired correctly.Import path and symbol align with Curve's named export. No issues.
25-25: ShapeType union extended — update docs/contracts.Adding 'curve' looks good. Please ensure prop docs for all consumers of Shape/ActiveShape mention 'curve' as a valid shapeType.
src/cartesian/Line.tsx (2)
90-90: Public/internal API extension for custom line shape — looks good.Typing ActiveShape<CurveProps, SVGPathElement> is appropriate and consistent with utilities.
Also applies to: 129-129
54-55: Using Shape wrapper is the right abstraction.Importing Shape from ActiveShapeUtils matches the shared pattern across components. No concerns.
| const { tick, tickInterval = 30, ...restProps } = props as CustomLineShape & CurveProps; | ||
| const { points } = restProps; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove 'as' assertion and type/name shadowing; type the component explicitly.
Current code uses as CustomLineShape & CurveProps and reuses the identifier for both the type and the component name. This violates the TS guidelines and hurts readability.
Apply this refactor:
- type CustomLineShape = {
+ type CustomLineShapeProps = {
tick: ReactElement;
tickInterval?: number;
};
- const CustomLineShape = (props: CustomLineShape) => {
- const { tick, tickInterval = 30, ...restProps } = props as CustomLineShape & CurveProps;
+ const CustomLineShape = (props: CurveProps & CustomLineShapeProps): ReactElement => {
+ const { tick, tickInterval = 30, ...restProps } = props;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In storybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsx around
lines 34 to 36, remove the usage of the 'as' type assertion and avoid reusing
the same identifier for the component and the type: give the component an
explicit props type in its function signature (e.g., use an interface or a union
type like CustomLineShapeProps & CurveProps), rename either the type or the
component to prevent name shadowing, and then destructure props normally (const
{ tick, tickInterval = 30, points, ...rest } = props) so TypeScript infers types
without assertions; ensure imports/types are updated to match the new names.
| let l = Math.abs(p1.x - p2.x); | ||
| const dx = (p2.x - p1.x) / l; | ||
| const dy = (p2.y - p1.y) / l; | ||
| const a = (Math.atan2(dy, dx) * 180) / Math.PI; | ||
|
|
||
| const tickCount = Math.abs(Math.floor(l / tickInterval - 1)); | ||
| const tickLength = l / tickCount; | ||
| let tickRemaining = tickInterval / 2; | ||
|
|
||
| let { x, y } = p1; | ||
| while (l - tickRemaining > 0) { | ||
| l -= tickRemaining; | ||
|
|
||
| x += dx * tickRemaining; | ||
| y += dy * tickRemaining; | ||
|
|
||
| ticks.push( | ||
| <g key={`${i}-${++counter}`} transform={`translate(${x} ${y}) rotate(${a})`}> | ||
| {tick} | ||
| </g>, | ||
| ); | ||
|
|
||
| tickRemaining = tickLength; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix segment math: handle vertical/zero-length segments; avoid divide‑by‑zero/NaN.
Using l = Math.abs(p1.x - p2.x) and normalizing by l breaks for vertical or zero‑length segments (NaN/Infinity), and tickCount = Math.abs(Math.floor(l / tickInterval - 1)) can yield 0 → division by zero. Compute Euclidean length, guard l <= 0, and step by a constant interval.
Apply this patch:
- let l = Math.abs(p1.x - p2.x);
- const dx = (p2.x - p1.x) / l;
- const dy = (p2.y - p1.y) / l;
- const a = (Math.atan2(dy, dx) * 180) / Math.PI;
-
- const tickCount = Math.abs(Math.floor(l / tickInterval - 1));
- const tickLength = l / tickCount;
- let tickRemaining = tickInterval / 2;
-
- let { x, y } = p1;
- while (l - tickRemaining > 0) {
- l -= tickRemaining;
-
- x += dx * tickRemaining;
- y += dy * tickRemaining;
+ const dx = p2.x - p1.x;
+ const dy = p2.y - p1.y;
+ const l = Math.hypot(dx, dy);
+ if (!Number.isFinite(l) || l <= 0) {
+ continue;
+ }
+ const a = (Math.atan2(dy, dx) * 180) / Math.PI;
+ const ux = dx / l; // unit vector
+ const uy = dy / l;
+ let offset = tickInterval / 2;
+ let counterLocal = 0;
+ while (offset < l) {
+ const x = p1.x + ux * offset;
+ const y = p1.y + uy * offset;
- ticks.push(
- <g key={`${i}-${++counter}`} transform={`translate(${x} ${y}) rotate(${a})`}>
+ ticks.push(
+ <g key={`${i}-${++counter}-${++counterLocal}`} transform={`translate(${x} ${y}) rotate(${a})`}>
{tick}
</g>,
);
-
- tickRemaining = tickLength;
+ offset += tickInterval;
}🤖 Prompt for AI Agents
In storybook/stories/Examples/LineChart/CustomLineShapeChart.stories.tsx around
lines 52 to 76, the segment math uses l = Math.abs(p1.x - p2.x) and divides by l
which fails for vertical or zero-length segments and tickCount computation can
become zero causing division-by-zero; replace the horizontal-only length with
the Euclidean distance between p1 and p2, guard early for l <= 0 by skipping the
segment, compute dx and dy as (p2.x - p1.x)/l and (p2.y - p1.y)/l after that
guard, determine tick stepping by iterating with a fixed tickInterval (or using
Math.floor(l / tickInterval) to get count but ensure count > 0) and avoid
dividing by tickCount when it could be zero, then advance points by tickInterval
along the normalized direction to push ticks until remaining length <
tickInterval.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
www/src/docs/api/Line.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
www/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Use the www directory to add and commit examples for the documentation website (recharts.github.io)
Files:
www/src/docs/api/Line.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Ensure code lints cleanly before submitting PRs (npm run lint)
Never use the TypeScript any type (implicit or explicit)
Prefer unknown over any and refine types appropriately
Explicitly type all function parameters and return values; do not rely on implicit any or inference
Do not use as type assertions; the only exception is as const
Files:
www/src/docs/api/Line.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
www/src/docs/api/Line.ts (1)
69-81: Past issue resolved; consider adding examples for consistency.The copy-paste error from the previous review has been corrected - the descriptions now properly reference "line" (线条) instead of "bar" (柱条). Good work!
For consistency with similar props like
dot,activeDot, andlabel, consider adding anexamplesarray that references the CustomLineShapeChart story or a documentation example.You could add an examples section like this:
format: ['<Line dataKey="value" shape={<CustomizedShape/>}/>', '<Line dataKey="value" shape={renderShape}/>'], + examples: [ + { + name: 'A line chart with customized shape', + url: '/examples/CustomLineShapeChart', + }, + ], },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cartesian/Line.tsx(7 hunks)www/src/docs/api/Line.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cartesian/Line.tsx
🧰 Additional context used
📓 Path-based instructions (2)
www/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Use the www directory to add and commit examples for the documentation website (recharts.github.io)
Files:
www/src/docs/api/Line.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Ensure code lints cleanly before submitting PRs (npm run lint)
Never use the TypeScript any type (implicit or explicit)
Prefer unknown over any and refine types appropriately
Explicitly type all function parameters and return values; do not rely on implicit any or inference
Do not use as type assertions; the only exception is as const
Files:
www/src/docs/api/Line.ts
| * | ||
| */ | ||
| type ShapeType = 'trapezoid' | 'rectangle' | 'sector' | 'symbols'; | ||
| type ShapeType = 'trapezoid' | 'rectangle' | 'sector' | 'symbols' | 'curve'; |
There was a problem hiding this comment.
Line is not the only user of the Shape, right? There's also Bar, Pie, RadialBar, Funnel, maybe others. What happens when one passes shape="curve" as a prop to Pie?
There was a problem hiding this comment.
Ha wait this is not external API, is it. That's hardcoded in Line, what you're after is the rest
export type ActiveShape<PropsType = Record<string, any>, ElementType = SVGElement> =
| ReactElement<SVGProps<ElementType>>
| ((props: PropsType) => ReactElement<SVGProps<ElementType>>)
| ((props: unknown) => JSX.Element)
| SVGProps<ElementType>
| boolean;
There was a problem hiding this comment.
so nothing to do here, right?
| dataKey="pv" | ||
| stroke="#8884d8" | ||
| activeDot={{ r: 8 }} | ||
| shape={<CustomLineShape tick={<circle r={5} fill="currentColor" />} />} |
There was a problem hiding this comment.
Element cloning was a mistake, please try to come up with an example code that passes a Component or a function instead of cloned element.
There was a problem hiding this comment.
wow, didn't that there is no createComponent anymore.. we are still using always using elements for custom shapes, gonna change that
There was a problem hiding this comment.
Is the current state of this code not passing a component?
There was a problem hiding this comment.
It's passing element. Which we later clone and pass extra props. I would prefer to see a component, no cloning required
There was a problem hiding this comment.
@PavelVanecek some offtop, are you going to deprecate passing ReactElement as shape?
and why is there this in ActiveShape:
| ((props: PropsType) => ReactElement<SVGProps<ElementType>>)
| ((props: unknown) => JSX.Element)
and not just (props: PropsType) => JSX.Element? cause i'm losing typings cause of that sometimes
There was a problem hiding this comment.
Yeah at some point we will start taking types seriously and that will force us to deprecate passing elements. The return type was added in good faith but it doesn't play well when actually used. Feel free to open another issue for it, we need to fix that later too.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6512 +/- ##
=======================================
Coverage 93.54% 93.54%
=======================================
Files 430 430
Lines 39096 39112 +16
Branches 4531 4532 +1
=======================================
+ Hits 36572 36588 +16
Misses 2509 2509
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 232 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
|
|
@PavelVanecek i suppose i should make my PR use "shape" instead of "content" so remain consistent |
|
Tooltip and Legend have content. We won't have consistency either way. |
|
Content for non-chart components, shape for chart components. At least mostly.. lol |
|
@PavelVanecek I updated the shape, now passing as fn |
|
thanks @tarik02 |
Description
I added
shapeprop to<Line>component, just like we have this prop in other components like<Bar>,<Area>, etc.Related Issue
#6511
Motivation and Context
This change is required because we need to customize lines on charts.
How Has This Been Tested?
I added storybook story, ran all existing test cases.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Documentation
Examples