[WEB-4855] refactor: bar chart improvements#7731
[WEB-4855] refactor: bar chart improvements#7731JayashTripathy wants to merge 3 commits intopreviewfrom
Conversation
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the bar chart component by adding support for custom axis ticks and fixing a missing fill property. The changes improve chart customization capabilities while ensuring proper default values.
- Adds customTicks property to allow custom X and Y axis tick components
- Implements proper fill property handling with fallback to default color
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/types/src/charts/index.ts | Adds customTicks type definition to TBarChartProps interface |
| packages/propel/src/charts/bar-chart/root.tsx | Implements custom tick support and adds fill property with default fallback |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| key={bar.key} | ||
| dataKey={bar.key} | ||
| stackId={bar.stackId} | ||
| fill={typeof bar.fill === "function" ? "#000000" : bar.fill} |
There was a problem hiding this comment.
The hardcoded fallback color '#000000' should be extracted to a constant or use a theme-based default color to maintain consistency with the design system.
packages/types/src/charts/index.ts
Outdated
| customTicks?: { | ||
| x?: React.ComponentType<any>; | ||
| y?: React.ComponentType<any>; | ||
| }; |
There was a problem hiding this comment.
Using 'any' type weakens type safety. Consider defining a proper interface for tick component props or use a more specific type constraint.
JayashTripathy
changed the title
JayashTripathy
changed the title
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces optional customTicks prop to Area, Bar, Line, and Scatter charts to select axis tick components at render time. Refactors chart prop types: TChartProps renamed to TBaseChartProps, adds TAxisChartProps with customTicks and tickCount, and updates chart-specific prop types. Adds and uses DEFAULT_BAR_FILL_COLOR in bar chart rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Chart as Chart (Area/Bar/Line/Scatter)
participant AxisX as XAxis
participant AxisY as YAxis
participant TickX as TickComponent (X)
participant TickY as TickComponent (Y)
App->>Chart: render({ customTicks, xAxis, yAxis, ... })
Chart->>AxisX: render with tick = customTicks?.x || CustomXAxisTick
AxisX->>TickX: render(props)
Chart->>AxisY: render with tick = customTicks?.y || CustomYAxisTick
AxisY->>TickY: render(props)
note over Chart,AxisY: Backward compatible when customTicks not provided
sequenceDiagram
autonumber
participant App as App
participant BarRoot as BarChart/root
participant Bar as CustomBar/CustomBarLollipop
App->>BarRoot: render({ bars, ... })
BarRoot->>Bar: render({ fill })
alt fill is function at render time
Bar-->>Bar: use DEFAULT_BAR_FILL_COLOR "#6B7280"
else fill is color/string
Bar-->>Bar: use provided fill
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/propel/src/charts/area-chart/root.tsx (1)
83-103: Fix divide-by-zero when data has a single point.
index / (data.length - 1)yieldsNaNforlength === 1, producing invalid values forcomparisonLine.- const comparisonLineData = useMemo(() => { - if (!data || data.length === 0) return []; + const comparisonLineData = useMemo(() => { + if (!data?.length) return []; + if (data.length === 1) { + return data.map((item) => ({ + [xAxis.key]: item[xAxis.key], + comparisonLine: 0, + })); + } // get the last data point const lastPoint = data[data.length - 1]; // for the y-value in the last point, use its yAxis key value const lastYValue = lastPoint[yAxis.key] ?? 0; // create data for a straight line that has points at each x-axis position return data.map((item, index) => { // calculate the y value for this point on the straight line // using linear interpolation between (0,0) and (last_x, last_y) - const ratio = index / (data.length - 1); + const ratio = index / (data.length - 1); const interpolatedValue = ratio * lastYValue;packages/propel/src/charts/bar-chart/bar.tsx (1)
166-176: Pass through original fill to shapes to preserve function-based coloring.Currently
createShapeVariantresolvesfillto a string, making the above fixes unreachable.- fill={typeof bar.fill === "function" ? bar.fill(shapeProps.payload) : bar.fill} + fill={bar.fill as any}
♻️ Duplicate comments (3)
packages/types/src/charts/index.ts (2)
24-31: Tighten tooltip prop typing (avoidany).Recommend narrowing to safer primitives to reduce unsoundness without pulling Recharts types into the types package.
- customTooltipContent?: (props: { active?: boolean; label: string; payload: any }) => React.ReactNode; + customTooltipContent?: (props: { active?: boolean; label: string | number; payload: unknown[] }) => React.ReactNode;
33-58: TypecustomTicksmore strictly or document the contract.
React.ComponentType<any>hides the expected tick props. Minimum: switch tounknown. Better: export explicit tick prop interfaces (e.g.,{ x: number; y: number; payload: { value: string | number } }), and use them for x/y.- customTicks?: { - x?: React.ComponentType<any>; - y?: React.ComponentType<any>; - }; + customTicks?: { + x?: React.ComponentType<unknown>; + y?: React.ComponentType<unknown>; + };Optionally add explicit types (outside this hunk):
export type TXAxisTickProps = { x: number; y: number; payload: { value: string | number }; getLabel?: (v: string | number) => React.ReactNode }; export type TYAxisTickProps = { x: number; y: number; payload: { value: string | number } };packages/propel/src/charts/bar-chart/root.tsx (1)
52-57: Unify fallback color with design token.Use
DEFAULT_BAR_FILL_COLORinstead of hardcoded#000000for tooltip dot colors.- colors[bar.key] = typeof bar.fill === "function" ? "#000000" : bar.fill; + colors[bar.key] = typeof bar.fill === "function" ? DEFAULT_BAR_FILL_COLOR : bar.fill;
🧹 Nitpick comments (4)
packages/types/src/charts/index.ts (1)
78-81: Alignfillrequirement with runtime fallback
Runtime appliesDEFAULT_BAR_FILL_COLORwhenfillis a function (seepackages/propel/src/charts/bar-chart/bar.tsx:13). Either makeTBarItem.filloptional (fill?: string | ((payload: T) => string)) inpackages/types/src/charts/index.ts, or remove the fallback and keepfillrequired.packages/propel/src/charts/area-chart/root.tsx (1)
117-131: Honor x-axis labeldyfrom props.The types expose
xAxis.dy, but the label uses a fixed28. Use the provided value when present.- label={ + label={ xAxis.label && { value: xAxis.label, - dy: 28, + dy: xAxis.dy ?? 28, className: AXIS_LABEL_CLASSNAME, } }packages/propel/src/charts/scatter-chart/root.tsx (2)
96-101: Honor x-axis labeldyfrom props.Use
xAxis.dyif provided.- xAxis.label && { + xAxis.label && { value: xAxis.label, - dy: 28, + dy: xAxis.dy ?? 28, className: AXIS_LABEL_CLASSNAME, }
109-116: RespectyAxis.offsetconsistently (matches AreaChart).Offset is hardcoded to
-24. Use the configured value.- yAxis.label && { + yAxis.label && { value: yAxis.label, angle: -90, position: "bottom", - offset: -24, + offset: yAxis.offset ?? -24, dx: yAxis.dx ?? -16, className: AXIS_LABEL_CLASSNAME, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/propel/src/charts/area-chart/root.tsx(3 hunks)packages/propel/src/charts/bar-chart/bar.tsx(4 hunks)packages/propel/src/charts/bar-chart/root.tsx(5 hunks)packages/propel/src/charts/line-chart/root.tsx(3 hunks)packages/propel/src/charts/scatter-chart/root.tsx(3 hunks)packages/types/src/charts/index.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/propel/src/charts/line-chart/root.tsx (1)
packages/propel/src/charts/components/tick.tsx (2)
CustomXAxisTick(7-13)CustomYAxisTick(16-22)
packages/propel/src/charts/scatter-chart/root.tsx (1)
packages/propel/src/charts/components/tick.tsx (2)
CustomXAxisTick(7-13)CustomYAxisTick(16-22)
packages/propel/src/charts/bar-chart/root.tsx (2)
packages/propel/src/charts/bar-chart/bar.tsx (1)
DEFAULT_BAR_FILL_COLOR(180-180)packages/propel/src/charts/components/tick.tsx (2)
CustomXAxisTick(7-13)CustomYAxisTick(16-22)
packages/propel/src/charts/area-chart/root.tsx (1)
packages/propel/src/charts/components/tick.tsx (2)
CustomXAxisTick(7-13)CustomYAxisTick(16-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
packages/types/src/charts/index.ts (5)
98-101: LGTM on Line chart props extension.
Consistent withTAxisChartProps.
113-116: LGTM on Scatter chart props extension.
Type composition reads clean.
134-140: LGTM on Area chart props.
comparisonLineshape is clear and minimal.
151-153: LGTM on Pie props pick.
Correct reuse of base props.
234-235: LGTM on Radar props pick.
Consistent with refactor.packages/propel/src/charts/area-chart/root.tsx (1)
118-121: Custom tick injection looks good.Dynamic selection preserves backward compatibility with existing Custom* ticks.
Also applies to: 147-150
packages/propel/src/charts/scatter-chart/root.tsx (2)
89-92: Custom X tick injection is correct.Runtime selection between default and custom components is clean.
118-121: Custom Y tick injection is correct.
Matches X axis pattern; no issues.packages/propel/src/charts/line-chart/root.tsx (2)
35-35: Prop addition looks good and is backward-compatible.
133-136: Dynamic Y-axis tick component: LGTM.packages/propel/src/charts/bar-chart/bar.tsx (2)
13-13: Good: centralized default bar color.
180-181: Exporting the default color: LGTM.packages/propel/src/charts/bar-chart/root.tsx (4)
22-22: Import of DEFAULT_BAR_FILL_COLOR: LGTM.
38-38: Prop addition for custom ticks: LGTM.
69-69: OK to set Bar fill; shapes will override.No action needed; just ensure it stays in sync with the default color constant.
126-129: Dynamic Y-axis tick component: LGTM.
| d={getBarPath(x, y, width, height, topBorderRadius, bottomBorderRadius)} | ||
| className="transition-opacity duration-200" | ||
| fill={typeof fill === "function" ? fill(payload) : fill} | ||
| fill={typeof fill === "function" ? DEFAULT_BAR_FILL_COLOR : fill} | ||
| opacity={opacity} | ||
| /> |
There was a problem hiding this comment.
Bug: functional fills are ignored; bars won’t use per-datapoint color.
When fill is a function, this should call fill(payload), not a static default.
- fill={typeof fill === "function" ? DEFAULT_BAR_FILL_COLOR : fill}
+ fill={typeof fill === "function" ? fill(payload) : fill}📝 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.
| d={getBarPath(x, y, width, height, topBorderRadius, bottomBorderRadius)} | |
| className="transition-opacity duration-200" | |
| fill={typeof fill === "function" ? fill(payload) : fill} | |
| fill={typeof fill === "function" ? DEFAULT_BAR_FILL_COLOR : fill} | |
| opacity={opacity} | |
| /> | |
| d={getBarPath(x, y, width, height, topBorderRadius, bottomBorderRadius)} | |
| className="transition-opacity duration-200" | |
| fill={typeof fill === "function" ? fill(payload) : fill} | |
| opacity={opacity} | |
| /> |
🤖 Prompt for AI Agents
In packages/propel/src/charts/bar-chart/bar.tsx around lines 110 to 114, the
current fill prop uses a static default when fill is a function; update it so
when fill is a function it is invoked with the bar's payload (e.g.
fill(payload)) and its return value is used as the fill; keep the existing
behavior of using the provided non-function fill value unchanged and fallback to
DEFAULT_BAR_FILL_COLOR only if the function returns a falsy/undefined value.
| cx={x + width / 2} | ||
| cy={y} | ||
| r={DEFAULT_LOLLIPOP_CIRCLE_RADIUS} | ||
| fill={typeof fill === "function" ? fill(payload) : fill} | ||
| fill={typeof fill === "function" ? DEFAULT_BAR_FILL_COLOR : fill} | ||
| stroke="none" | ||
| /> |
There was a problem hiding this comment.
Circle color inconsistent with lollipop stroke.
Circle should match computed stroke color when fill is a function.
- fill={typeof fill === "function" ? DEFAULT_BAR_FILL_COLOR : fill}
+ fill={typeof fill === "function" ? fill(payload) : fill}📝 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.
| cx={x + width / 2} | |
| cy={y} | |
| r={DEFAULT_LOLLIPOP_CIRCLE_RADIUS} | |
| fill={typeof fill === "function" ? fill(payload) : fill} | |
| fill={typeof fill === "function" ? DEFAULT_BAR_FILL_COLOR : fill} | |
| stroke="none" | |
| /> | |
| cx={x + width / 2} | |
| cy={y} | |
| r={DEFAULT_LOLLIPOP_CIRCLE_RADIUS} | |
| fill={typeof fill === "function" ? fill(payload) : fill} | |
| stroke="none" | |
| /> |
🤖 Prompt for AI Agents
In packages/propel/src/charts/bar-chart/bar.tsx around lines 140 to 145, the
circle's fill uses DEFAULT_BAR_FILL_COLOR when fill is a function which makes it
inconsistent with the lollipop stroke; instead compute the stroke color exactly
the same way the stroke prop is computed (e.g., call fill(...) or the stroke
color resolver used for the lollipop) and set the circle's fill to that computed
stroke color when fill is a function, otherwise keep using the provided fill
value; ensure the fallback behavior for non-function fills remains unchanged.
| tick={(props) => { | ||
| const TickComponent = customTicks?.x || CustomXAxisTick; | ||
| return <TickComponent {...props} />; | ||
| }} |
There was a problem hiding this comment.
Regression: X-axis tick formatter (getLabel) dropped.
Same issue as LineChart; pass through xAxis.getLabel.
- tick={(props) => {
- const TickComponent = customTicks?.x || CustomXAxisTick;
- return <TickComponent {...props} />;
- }}
+ tick={(props) => {
+ const TickComponent = customTicks?.x || CustomXAxisTick;
+ return <TickComponent {...props} getLabel={xAxis.getLabel} />;
+ }}📝 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.
| tick={(props) => { | |
| const TickComponent = customTicks?.x || CustomXAxisTick; | |
| return <TickComponent {...props} />; | |
| }} | |
| tick={(props) => { | |
| const TickComponent = customTicks?.x || CustomXAxisTick; | |
| return <TickComponent {...props} getLabel={xAxis.getLabel} />; | |
| }} |
🤖 Prompt for AI Agents
In packages/propel/src/charts/bar-chart/root.tsx around lines 101 to 104, the
X-axis TickComponent is rendered without forwarding the axis label formatter
(xAxis.getLabel), which regresses behavior; update the tick render to pass
through the xAxis.getLabel (e.g. include getLabel: xAxis.getLabel in the props
spread passed to TickComponent) so custom and default ticks receive the label
formatter while keeping all existing props intact.
| tick={(props) => { | ||
| const TickComponent = customTicks?.x || CustomXAxisTick; | ||
| return <TickComponent {...props} />; | ||
| }} |
There was a problem hiding this comment.
Regression: X-axis tick formatter (getLabel) no longer applied.
CustomXAxisTick expects getLabel; without passing it, labels won’t be formatted.
Apply:
- tick={(props) => {
- const TickComponent = customTicks?.x || CustomXAxisTick;
- return <TickComponent {...props} />;
- }}
+ tick={(props) => {
+ const TickComponent = customTicks?.x || CustomXAxisTick;
+ return <TickComponent {...props} getLabel={xAxis.getLabel} />;
+ }}📝 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.
| tick={(props) => { | |
| const TickComponent = customTicks?.x || CustomXAxisTick; | |
| return <TickComponent {...props} />; | |
| }} | |
| tick={(props) => { | |
| const TickComponent = customTicks?.x || CustomXAxisTick; | |
| return <TickComponent {...props} getLabel={xAxis.getLabel} />; | |
| }} |
🤖 Prompt for AI Agents
In packages/propel/src/charts/line-chart/root.tsx around lines 104 to 107, the
tick renderer calls the TickComponent but does not pass the getLabel formatter
through, so CustomXAxisTick never receives getLabel; update the tick callback to
pass getLabel (preferably from customTicks.x if present, otherwise from the
current component props) along with the existing props so the TickComponent
receives and applies the label formatter.
Description
This PR adds the following :-
Type of Change
Summary by CodeRabbit