add bar chart and pie chart #72
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
WalkthroughThis pull request introduces two new chart components—BarChart and PieChart—each with full Storybook support and accompanying examples. The components, along with their TypeScript interfaces, are defined to accept various data formats and configuration options. Additionally, new index files re-export the components and their types, and the main UI index has been updated to include these exports. Changes
Sequence Diagram(s)sequenceDiagram
participant SB as Storybook
participant BC as BarChart Component
participant RC as Recharts Library
SB->>BC: Render BarChart with props (data, configurations)
BC->>BC: Transform input data to chartData format
BC->>RC: Invoke rendering of Bar elements (axes, tooltips, legend)
RC-->>SB: Return rendered BarChart
sequenceDiagram
participant SB as Storybook
participant PC as PieChart Component
participant RC as Recharts Library
SB->>PC: Render PieChart with props (data, title, total)
PC->>PC: Process data and assign default colors
PC->>RC: Call rendering of Pie, Legend, and label (if total provided)
RC-->>SB: Return rendered PieChart
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/ui/src/components/BarChart/BarChart.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/packages/ui".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "packages/ui/package.json". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
packages/ui/src/components/PieChart/PieChart.stories.tsx (2)
70-83: Consider adding a visible indication for the NoValues story.While the NoValues story is appropriately named and described, the actual implementation might not be immediately clear from the story args. Consider adding a
showValues: falseprop to make it more explicit how this functionality is controlled.args: { width: 800, data: [ { name: "Category A", value: 30 }, { name: "Category B", value: 20 }, { name: "Category C", value: 15 }, { name: "Category D", value: 25 }, { name: "Category E", value: 10 }, ], title: "No Values", description: "Hiding value labels", + showValues: false, },
85-98: Legend control isn't explicitly shown in WithLegend story.Similar to the NoValues story, it's not clear from the args how the legend is actually enabled. Consider adding a
showLegend: trueprop to make it more explicit.args: { width: 800, data: [ { name: "Category A", value: 30 }, { name: "Category B", value: 20 }, { name: "Category C", value: 15 }, { name: "Category D", value: 25 }, { name: "Category E", value: 10 }, ], title: "Distribution with Legend", description: "Showing data distribution with a legend", + showLegend: true, },packages/ui/src/components/BarChart/BarChart.tsx (1)
96-161: Add responsive width handlingThe chart container has a fixed height of 400px but doesn't adjust width responsively. This may cause rendering issues on different screen sizes.
- <ChartContainer config={chartConfig} className="mx-auto aspect-auto h-[400px] w-full"> + <ChartContainer config={chartConfig} className="mx-auto aspect-auto h-[400px] w-full lg:w-auto">Also, consider adding a
responsiveprop to theRechartsBarChartcomponent to make it adapt to its container:<RechartsBarChart data={chartData} + responsive={true} margin={{ top: 20, right: 30, left: 20, bottom: 60, }} >packages/ui/src/components/PieChart/PieChart.tsx (2)
8-30: Consider extracting color constantsThe
DEFAULT_COLORSarray is quite large. If it will be used across multiple components, consider extracting it to a shared constants file.You could move this to something like
packages/ui/src/constants/colors.ts:export const DEFAULT_CHART_COLORS = [ "#D9D6FF", "#B3B0FF", // ...rest of colors ];Then import it with:
+ import { DEFAULT_CHART_COLORS } from "../../constants/colors"; - const DEFAULT_COLORS = [ - "#D9D6FF", - "#B3B0FF", - // ...etc - ];
86-106: Handle potential unexpected valuesThe component doesn't handle cases where data items might have zero or negative values. Consider adding validation or handling for these edge cases.
You could add validation to ensure that the chart displays correctly even with zero or negative values:
<Pie data={chartData} dataKey="value" nameKey="name" - label + label={({value}) => value > 0 ? value.toLocaleString() : ''} outerRadius={total ? 120 : 140} innerRadius={total ? 60 : 0} paddingAngle={total ? 2 : 0} >packages/ui/src/components/BarChart/BarChart.stories.tsx (1)
28-40: Ensure consistent data generation for time seriesThe current implementation generates random values for each data point, which makes it difficult to visually test and verify the component behavior consistently.
Consider using deterministic values or a seeded random generator for more predictable story rendering:
function generateTimeSeriesData(days = 30) { const data = []; const now = new Date(); + // Use a seeded value for consistent random generation + let seed = 42; + const random = () => { + const x = Math.sin(seed++) * 10000; + return Math.floor((x - Math.floor(x)) * 1000) + 500; + }; for (let i = days - 1; i >= 0; i--) { const date = new Date(now); date.setDate(date.getDate() - i); data.push({ x: date.toISOString().split("T")[0], - y: Math.floor(Math.random() * 1000) + 500, + y: random(), }); } return data; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/ui/src/components/BarChart/BarChart.stories.tsx(1 hunks)packages/ui/src/components/BarChart/BarChart.tsx(1 hunks)packages/ui/src/components/BarChart/index.ts(1 hunks)packages/ui/src/components/PieChart/PieChart.stories.tsx(1 hunks)packages/ui/src/components/PieChart/PieChart.tsx(1 hunks)packages/ui/src/components/PieChart/index.ts(1 hunks)packages/ui/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/ui/src/components/BarChart/BarChart.tsx (1)
packages/ui/src/components/BarChart/index.ts (2)
BarChartProps(2-2)BarChart(1-1)
packages/ui/src/components/BarChart/BarChart.stories.tsx (1)
packages/ui/src/components/BarChart/BarChart.tsx (1)
BarChart(36-165)
packages/ui/src/components/PieChart/PieChart.tsx (1)
packages/ui/src/components/PieChart/index.ts (2)
PieChartProps(2-2)PieChart(1-1)
🪛 Biome (1.9.4)
packages/ui/src/components/BarChart/BarChart.tsx
[error] 63-63: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/ui/src/components/PieChart/PieChart.tsx
[error] 63-63: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (9)
packages/ui/src/components/BarChart/index.ts (1)
1-2: Barrel file export pattern implemented correctly.This follows the standard barrel file pattern for exporting the component and its props type, which aligns with the project's existing structure.
packages/ui/src/components/PieChart/index.ts (1)
1-2: Barrel file export pattern implemented correctly.This follows the standard barrel file pattern for exporting the component and its props type, consistent with the project's structure.
packages/ui/src/index.ts (1)
45-46: Chart component exports added properly.The new exports for PieChart and BarChart components have been correctly added to the UI package's main index file, making them available to consumers of the package.
packages/ui/src/components/PieChart/PieChart.stories.tsx (5)
5-18: Storybook meta configuration looks good.The meta configuration for the PieChart stories follows Storybook best practices with appropriate title, component reference, and layout settings.
24-37: Basic story provides a good starting example.The Basic story provides a clear example with sample data that demonstrates the component's core functionality.
39-53: DonutChart story shows support for showing totals.This story effectively demonstrates the component's ability to display total values, which is a common use case for pie/donut charts.
55-68: CustomColors story demonstrates color customization.The story showcases how individual segments can have custom colors applied, enhancing the chart's visual customization options.
100-100: Export pattern follows Storybook conventions.The named exports of all story variants follows Storybook's recommended patterns.
packages/ui/src/components/BarChart/BarChart.tsx (1)
73-77: Check if input values are dates consistentlyThe component has a flag
isDateAxisbut also has a separateisDatefunction that checks if values are dates. These should work together consistently.Consider using the
isDateAxisflag to determine how to handle date parsing to avoid confusion:const isDate = (value: string) => { + if (!isDateAxis) return false; const date = new Date(value); return !isNaN(date.getTime()); };
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/ui/src/components/PieChart/PieChart.tsx (4)
64-73: Add explicit typing for the accumulator in the reduce functionThe reduce function doesn't have a type for the accumulator, which could lead to TypeScript not properly inferring the return type.
const chartConfig = data.reduce( - (acc, item, index) => { + (acc: Record<string, { label: string; color: string }>, item, index) => { acc[item.name] = { label: item.name, color: item.color || DEFAULT_COLORS[index % DEFAULT_COLORS.length], }; return acc; }, {}, );🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
58-73: Optimize data processing to avoid iterating over the data twiceCurrently, the data is processed twice - once for
chartDataand once forchartConfig. This could be optimized into a single pass.- const chartData = data.map((item, index) => ({ - name: item.name, - value: item.value, - fill: item.color || DEFAULT_COLORS[index % DEFAULT_COLORS.length], - })); - - const chartConfig = data.reduce( - (acc, item, index) => { - acc[item.name] = { - label: item.name, - color: item.color || DEFAULT_COLORS[index % DEFAULT_COLORS.length], - }; - return acc; - }, - {}, - ); + const chartData: { name: string; value: number; fill: string }[] = []; + const chartConfig: Record<string, { label: string; color: string }> = {}; + + data.forEach((item, index) => { + const color = item.color || DEFAULT_COLORS[index % DEFAULT_COLORS.length]; + + chartData.push({ + name: item.name, + value: item.value, + fill: color, + }); + + chartConfig[item.name] = { + label: item.name, + color: color, + }; + });🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
76-120: Enhance accessibility with ARIA attributesThe chart lacks ARIA attributes that would make it more accessible to users with screen readers.
Consider adding:
aria-labelto the chart containerrole="img"to the chartaria-describedbyto connect the chart with its descriptionExample implementation:
- <Card className="flex flex-col" style={{ width }}> + <Card className="flex flex-col" style={{ width }} aria-label={title || "Pie Chart"}> <CardHeader className="items-center pb-0"> {title && <CardTitle>{title}</CardTitle>} - {description && <CardDescription>{description}</CardDescription>} + {description && <CardDescription id="chart-description">{description}</CardDescription>} </CardHeader> <CardContent className="flex-1 pb-0"> - <div style={{ width: "100%", height: height }}> + <div + style={{ width: "100%", height: height }} + role="img" + aria-describedby={description ? "chart-description" : undefined} + >
87-117: Consider providing a way to customize the legend positionThe current implementation has a fixed legend position (right side, middle). For better flexibility, consider making the legend position configurable through props.
export interface PieChartProps { width?: number; height?: number; data: { name: string; value: number; color?: string; }[]; title?: string; description?: string; total?: string; totalLabel?: string; + legendPosition?: { + verticalAlign?: "top" | "middle" | "bottom"; + align?: "left" | "center" | "right"; + layout?: "horizontal" | "vertical"; + }; } // Then in the component: export function PieChart({ width = 500, height = 400, data, title, description, total, totalLabel = "Total", + legendPosition = { verticalAlign: "middle", align: "right", layout: "vertical" }, }: PieChartProps) { // And in the JSX: <Legend - verticalAlign="middle" - align="right" - layout="vertical" + verticalAlign={legendPosition.verticalAlign} + align={legendPosition.align} + layout={legendPosition.layout} wrapperStyle={{ paddingLeft: "20px" }} />packages/ui/src/components/BarChart/BarChart.tsx (3)
15-34: Ensure data consistency for each series'xandyarrays.Each series is assumed to have an aligned
xandyarray of the same length, but this constraint isn't explicitly validated inBarChartProps. For robustness, consider validating or documenting thatxandymust always match in length and that all series indatause the same set ofxvalues.
52-58: Check for multi-series alignment.This logic maps
xvalues from only the first series (data[0].x). While this is valid if all series share the samexarray, it could lead to misalignments if future usage allows series with distinct or misalignedxarrays. Consider either enforcing a singlexarray across all series or merging them safely if partial overlaps are expected.
72-77: Potential time zone handling.
new Date(value)relies on the environment's local time zone. If the dataset contains dates from multiple time zones, the displayed date may differ from the intended source. If needed, consider using a dedicated library for precise date handling or clarifying in docs that local time zone conversion is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/ui/src/components/BarChart/BarChart.stories.tsx(1 hunks)packages/ui/src/components/BarChart/BarChart.tsx(1 hunks)packages/ui/src/components/PieChart/PieChart.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/ui/src/components/BarChart/BarChart.tsx (1)
packages/ui/src/components/BarChart/index.ts (2)
BarChartProps(2-2)BarChart(1-1)
packages/ui/src/components/PieChart/PieChart.tsx (1)
packages/ui/src/components/PieChart/index.ts (2)
PieChartProps(2-2)PieChart(1-1)
packages/ui/src/components/BarChart/BarChart.stories.tsx (1)
packages/ui/src/components/BarChart/BarChart.tsx (1)
BarChart(36-165)
🪛 Biome (1.9.4)
packages/ui/src/components/BarChart/BarChart.tsx
[error] 62-62: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/ui/src/components/PieChart/PieChart.tsx
[error] 64-64: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (16)
packages/ui/src/components/BarChart/BarChart.tsx (5)
1-4: No concerns with module setup.The
"use client"directive and initial imports are straightforward, indicating that this component is intended for client-side rendering in a Next.js app. The setup looks good.
36-50: Props initialization receives a thumbs-up.Defaulting values for
width,height, andbarGap/barGroupGapis a good practice, ensuring consistent visuals even if a user omits these props. Everything looks fine here.
60-70: Good switch from spread syntax to direct assignment.By using direct object assignment in the
.reduce()callback, you avoid theO(n^2)performance pitfall flagged by static analysis tools. This is an efficient approach for building thechartConfig.🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
78-88: Format logic is straightforward.
formatXAxisappropriately toggles between date formatting and raw values based onisDateAxis. The fallback to returning the string for numeric data is concise and should work as intended.
89-164: Overall chart rendering looks clean.The structure of the card, grid, axes, tooltip, legend, and bar elements is logically organized. The usage of
ChartContainerandChartTooltipContentis consistent, making for a well-structured chart component. The code is readable and maintainable.packages/ui/src/components/BarChart/BarChart.stories.tsx (11)
1-4: Basic Storybook setup is clear.Imports from Storybook and React are concise. Tagging the component for autodocs is standard practice and keeps documentation organized.
5-24: Story metadata structure is well-defined.Setting up decorators to center the story in an 800px-wide container is a common approach for consistent previews. This metadata ensures a uniform layout for all stories.
27-40: Utility functions for data generation are straightforward.Both
generateTimeSeriesDataandgenerateNumericDataare neatly implemented. They produce random data for illustrative purposes in the stories, which is common for Storybook demos.
50-58: Reference sample data is well-chosen.The
dateDataarray provides a neat example showcasing a single series with simple labeled x-values. Great for a default scenario.
59-88: Props forDefaultandWithMultipleSeriesstories look good.They cover basic date-based usage, as well as the multi-series scenario. Everything is consistent with the underlying
BarChartprops.
90-106:WithNumericXAxisexemplifies numeric handling.Demonstrates how
isDateAxis = falsecan be used with numeric categories. The approach is in line with theBarChartprop design.
108-131:TimeSeriesexample is well-structured.It uses generated time series data, properly assigning x and y arrays for daily donations over 30 days. Intervals for labels are a nice addition.
133-169: Use of a single dataset array for multiple series.You've adopted a single generated dataset (
multipleSeriesData) for theMultipleSeriesstory, ensuring consistent x-values across series. This addresses earlier reviewer feedback on inconsistent data.
171-199: Quarterly categories look correct.
Categoriesstory demonstrates a scenario with textual quarters and different yearly series. It nicely highlights how the chart handles multiple label sets.
201-236:NumericAxisstory is improved.By reusing a single
numericDataarray for both series, you avoid mismatched indices. This aligns well with the best practice of consistent x-values per story.
238-265:NoLegendstory effectively shows a legend-free scenario.Again, good job using a shared dataset (
noLegendData) to eliminate potential mismatch. This final story thoroughly demonstrates different chart variations without the legend.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/ui/src/components/PieChart/PieChart.tsx (6)
88-89: Consider adding aria attributes for accessibilityThe chart doesn't have any accessibility attributes, which could make it difficult for screen reader users to understand the content.
- <RechartsPieChart width={width} height={height}> + <RechartsPieChart + width={width} + height={height} + aria-label={title || "Pie Chart"} + role="img">
90-98: Customize chart tooltip for better user experienceThe tooltip is using the default configuration, but you could enhance it to show both the value and its percentage of the total.
- <ChartTooltip content={<ChartTooltipContent hideLabel />} /> + <ChartTooltip + content={ + <ChartTooltipContent + hideLabel + formatter={(value, name, props) => { + const total = chartData.reduce((sum, item) => sum + item.value, 0); + const percent = ((value / total) * 100).toFixed(1); + return [`${value} (${percent}%)`, name]; + }} + /> + } + />
32-44: Consider additional configuration options for the pie chartThe interface is solid, but you might want to add options for animation, padding, and legend positioning that users might need.
export interface PieChartProps { width?: number; height?: number; data: { name: string; value: number; color?: string; }[]; title?: string; description?: string; total?: string; totalLabel?: string; + legendPosition?: 'right' | 'bottom' | 'left' | 'top'; + showAnimation?: boolean; + showPercentage?: boolean; }
59-63: Add type safety to the chartData transformationThe current implementation lacks explicit typing for the transform operation, which would improve type safety and code readability.
+ interface ChartDataItem { + name: string; + value: number; + fill: string; + } - const chartData = data.map((item, index) => ({ + const chartData: ChartDataItem[] = data.map((item, index) => ({ name: item.name, value: item.value, fill: item.color || DEFAULT_COLORS[index % DEFAULT_COLORS.length], }));
108-109: Consider fallback styles for className not workingThe className uses square bracket notation for styling which relies on Tailwind's JIT compiler. Consider adding fallback styles to ensure compatibility across environments.
className="[&>tspan:nth-child(2)]:text-xl [&>tspan:nth-child(2)]:font-bold" + style={{ + // Fallback styles if the className doesn't apply + fontFamily: "sans-serif" + }}
111-116: Make legend position configurable through propsCurrently, the legend position is hard-coded. Consider making it configurable through component props to increase flexibility.
// In the props interface + legendPosition?: 'right' | 'bottom' | 'left' | 'top'; + legendAlign?: 'center' | 'left' | 'right'; // In the component props + legendPosition = 'right', + legendAlign = 'center', // In the Legend component <Legend - verticalAlign="middle" - align="right" - layout="vertical" + verticalAlign={legendPosition === 'top' || legendPosition === 'bottom' ? legendPosition : 'middle'} + align={legendPosition === 'left' || legendPosition === 'right' ? legendPosition : legendAlign} + layout={legendPosition === 'left' || legendPosition === 'right' ? 'vertical' : 'horizontal'} wrapperStyle={{ paddingLeft: "20px" }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/PieChart/PieChart.tsx(1 hunks)
🔇 Additional comments (3)
packages/ui/src/components/PieChart/PieChart.tsx (3)
55-58: Good fix on returning null for empty data!Changing the return value from an empty array to
nullfor the empty data case is the correct approach. Empty arrays are not valid React nodes and can cause rendering issues.
65-74: Well implemented chartConfig without spread operator!The implementation avoids using the spread operator in the accumulator, which prevents O(n²) time complexity for large datasets. This is more performant than spreading objects in the reducer.
83-83: Good implementation of responsive container!Adding
maxWidth: "100%"andoverflow: "auto"improves the chart's responsiveness for different screen sizes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/ui/src/components/BarChart/BarChart.stories.tsx (4)
114-117: 🛠️ Refactor suggestionUse consistent data for the TimeSeries example.
Generating separate random data for x and y values can lead to inconsistent data points which could confuse users of the component.
+ const timeSeriesData = generateTimeSeriesData(30); { name: "Donations", - x: generateTimeSeriesData(30).map((d) => d.x), - y: generateTimeSeriesData(30).map((d) => d.y), + x: timeSeriesData.map((d) => d.x), + y: timeSeriesData.map((d) => d.y), color: "#25BDCE", },🧰 Tools
🪛 Biome (1.9.4)
[error] 114-115: expected
:but instead foundtimeSeriesDataRemove timeSeriesData
(parse)
[error] 115-115: expected
,but instead found;Remove ;
(parse)
[error] 116-116: expected
,but instead found(Remove (
(parse)
[error] 117-117: expected
,but instead found:Remove :
(parse)
137-145: 🛠️ Refactor suggestionUse consistent data for the MultipleSeries example.
Similar to the TimeSeries story, the MultipleSeries story also generates separate random data which can lead to inconsistencies between series.
+ const multipleSeriesData = generateTimeSeriesData(30); { name: "Donations", - x: generateTimeSeriesData(30).map((d) => d.x), - y: generateTimeSeriesData(30).map((d) => d.y), + x: multipleSeriesData.map((d) => d.x), + y: multipleSeriesData.map((d) => d.y), color: "#25BDCE", }, { name: "Matching Funds", - x: generateTimeSeriesData(30).map((d) => d.x), - y: generateTimeSeriesData(30).map((d) => d.y * 0.5), + x: multipleSeriesData.map((d) => d.x), + y: multipleSeriesData.map((d) => d.y * 0.5), color: "#FF7043", },🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '+'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
[error] 138-138: Expected an expression but instead found 'const'.
Expected an expression here.
(parse)
[error] 139-139: expected
,but instead found(Remove (
(parse)
[error] 139-139: expected
,but instead found;Remove ;
(parse)
[error] 142-142: expected
,but instead found(Remove (
(parse)
[error] 143-143: expected
,but instead found:Remove :
(parse)
[error] 143-143: expected
,but instead found(Remove (
(parse)
[error] 144-144: expected
,but instead found:Remove :
(parse)
[error] 144-144: expected
,but instead found(Remove (
(parse)
[error] 144-144: expected
,but instead found:Remove :
(parse)
[error] 145-145: expected
,but instead found(Remove (
(parse)
197-205: 🛠️ Refactor suggestionUse consistent data for the NumericAxis example.
The NumericAxis story generates separate random data for both series, which could lead to inconsistent x-axis values.
+ const numericData = generateNumericData(10); { name: "Series A", - x: generateNumericData(10).map((d) => d.x), - y: generateNumericData(10).map((d) => d.y), + x: numericData.map((d) => d.x), + y: numericData.map((d) => d.y), color: "#25BDCE", }, { name: "Series B", - x: generateNumericData(10).map((d) => d.x), - y: generateNumericData(10).map((d) => d.y * 0.7), + x: numericData.map((d) => d.x), + y: numericData.map((d) => d.y * 0.7), // Creating variation between series color: "#FF7043", },🧰 Tools
🪛 Biome (1.9.4)
[error] 204-204: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '+'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
[error] 204-204: Expected an expression but instead found 'const'.
Expected an expression here.
(parse)
227-229: 🛠️ Refactor suggestionUse consistent data for the NoLegend example.
The NoLegend story also generates separate random data for x and y values, which could lead to inconsistencies.
+ const noLegendData = generateTimeSeriesData(30); { name: "Donations", - x: generateTimeSeriesData(30).map((d) => d.x), - y: generateTimeSeriesData(30).map((d) => d.y), + x: noLegendData.map((d) => d.x), + y: noLegendData.map((d) => d.y), color: "#25BDCE", },🧰 Tools
🪛 Biome (1.9.4)
[error] 228-228: expected
,but instead found:Remove :
(parse)
🧹 Nitpick comments (3)
packages/ui/src/components/PieChart/PieChart.tsx (2)
89-109: Consider adding fallback for empty values in pie segments.The Pie component doesn't handle zero-value segments differently. For improved UX, consider filtering out zero values or implementing a special case visualization.
<Pie data={chartData} + data={chartData.filter(item => item.value > 0)} dataKey="value" nameKey="name" label outerRadius={total ? 120 : 140} innerRadius={total ? 60 : 0} paddingAngle={total ? 2 : 0} >
110-115: Consider making legend position configurable.The Legend component has fixed positioning with hardcoded alignment values. Consider making this configurable through props for more flexible use cases.
<Legend - verticalAlign="middle" - align="right" - layout="vertical" + verticalAlign={legendPosition?.vertical || "middle"} + align={legendPosition?.horizontal || "right"} + layout={legendPosition?.layout || "vertical"} wrapperStyle={{ paddingLeft: "20px" }} />This would require adding new props to the PieChartProps interface:
legendPosition?: { vertical?: "top" | "middle" | "bottom"; horizontal?: "left" | "center" | "right"; layout?: "horizontal" | "vertical"; };packages/ui/src/components/BarChart/BarChart.stories.tsx (1)
28-40: Consider adding a seed parameter to generateTimeSeriesData for consistent testing.Adding an optional seed parameter to the random data generation would allow for reproducible examples and consistent testing.
- function generateTimeSeriesData(days = 30) { + function generateTimeSeriesData(days = 30, seed = Math.random()) { const data = []; const now = new Date(); + const random = (min: number, max: number) => { + const x = Math.sin(seed++) * 10000; + return Math.floor((x - Math.floor(x)) * (max - min + 1)) + min; + }; for (let i = days - 1; i >= 0; i--) { const date = new Date(now); date.setDate(date.getDate() - i); data.push({ x: date.toISOString().split("T")[0], - y: Math.floor(Math.random() * 1000) + 500, + y: random(500, 1500), }); } return data; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/components/BarChart/BarChart.stories.tsx(1 hunks)packages/ui/src/components/PieChart/PieChart.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/components/PieChart/PieChart.tsx
[error] 56-56: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 57-58: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 56-58: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/ui/src/components/BarChart/BarChart.stories.tsx
[error] 114-115: expected : but instead found timeSeriesData
Remove timeSeriesData
(parse)
[error] 115-115: expected , but instead found ;
Remove ;
(parse)
[error] 116-116: expected , but instead found (
Remove (
(parse)
[error] 117-117: expected , but instead found :
Remove :
(parse)
[error] 119-119: expected , but instead found (
Remove (
(parse)
[error] 120-120: expected , but instead found :
Remove :
(parse)
[error] 138-138: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '+'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
[error] 138-138: Expected an expression but instead found 'const'.
Expected an expression here.
(parse)
[error] 139-139: expected , but instead found (
Remove (
(parse)
[error] 139-139: expected , but instead found ;
Remove ;
(parse)
[error] 142-142: expected , but instead found (
Remove (
(parse)
[error] 143-143: expected , but instead found :
Remove :
(parse)
[error] 143-143: expected , but instead found (
Remove (
(parse)
[error] 144-144: expected , but instead found :
Remove :
(parse)
[error] 144-144: expected , but instead found (
Remove (
(parse)
[error] 144-144: expected , but instead found :
Remove :
(parse)
[error] 145-145: expected , but instead found (
Remove (
(parse)
[error] 148-148: expected , but instead found :
Remove :
(parse)
[error] 149-149: expected , but instead found (
Remove (
(parse)
[error] 149-149: expected , but instead found :
Remove :
(parse)
[error] 150-150: expected , but instead found (
Remove (
(parse)
[error] 151-151: expected , but instead found :
Remove :
(parse)
[error] 152-152: expected , but instead found (
Remove (
(parse)
[error] 153-153: expected , but instead found :
Remove :
(parse)
[error] 153-153: expected , but instead found (
Remove (
(parse)
[error] 155-155: expected , but instead found :
Remove :
(parse)
[error] 204-204: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '+'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
[error] 204-204: Expected an expression but instead found 'const'.
Expected an expression here.
(parse)
[error] 206-206: expected , but instead found (
Remove (
(parse)
[error] 206-206: expected , but instead found ;
Remove ;
(parse)
[error] 209-209: expected , but instead found (
Remove (
(parse)
[error] 209-209: expected , but instead found :
Remove :
(parse)
[error] 209-209: expected , but instead found (
Remove (
(parse)
[error] 210-210: expected , but instead found :
Remove :
(parse)
[error] 211-211: expected , but instead found (
Remove (
(parse)
[error] 212-212: expected , but instead found :
Remove :
(parse)
[error] 212-212: expected , but instead found (
Remove (
(parse)
[error] 213-213: expected , but instead found :
Remove :
(parse)
[error] 216-217: expected , but instead found (
Remove (
(parse)
[error] 220-220: expected , but instead found :
Remove :
(parse)
[error] 221-221: expected , but instead found (
Remove (
(parse)
[error] 223-223: expected , but instead found :
Remove :
(parse)
[error] 224-224: expected , but instead found (
Remove (
(parse)
[error] 226-226: expected , but instead found :
Remove :
(parse)
[error] 226-226: expected , but instead found (
Remove (
(parse)
[error] 228-228: expected , but instead found :
Remove :
(parse)
🔇 Additional comments (3)
packages/ui/src/components/PieChart/PieChart.tsx (3)
55-57: Good implementation of proper error handling for empty data.The early return for null or empty data is properly implemented, ensuring the component gracefully handles cases where no data is available for visualization.
🧰 Tools
🪛 Biome (1.9.4)
[error] 56-56: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
64-73: Good optimization using object assignment instead of spread.The chartConfig creation properly implements the recommended performance optimization by using direct object assignment instead of spread syntax in the reduce method's accumulator, avoiding O(n²) time complexity for large datasets.
82-82: Good implementation of responsive container.The responsive container with
maxWidth: "100%"andoverflow: "auto"ensures the chart adapts well to different screen sizes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/ui/src/components/BarChart/BarChart.tsx (4)
36-45: Unusedheightprop.
The function signature acceptsheightbut never applies it. Either remove the prop or apply it (e.g., in the chart container style).-export function BarChart({ - width = 800, - data, - title, - description, - xAxisTitle, - yAxisTitle, - showLegend = true, - isDateAxis = false, -}: BarChartProps) { +export function BarChart({ + width = 800, + height = 400, + data, + title, + description, + xAxisTitle, + yAxisTitle, + showLegend = true, + isDateAxis = false, +}: BarChartProps) { + ... + <Card className="flex flex-col" style={{ width, height }}> + ... + </Card> }
58-65: Remove unusedindexvariable in the reducer.
Theindexparameter is unused. For clarity, consider removing it.-const chartConfig = data.reduce<ChartConfig>((acc, series, index) => { +const chartConfig = data.reduce<ChartConfig>((acc, series) => { acc[series.name || "value"] = { label: series.name || "Value", color: series.color || "#25BDCE", }; return acc; }, {});
67-71: Consider removing or fully enablingisDate.
Currently,isDateis only invoked in commented-out code. Clear up whether it’s needed or can be removed.
147-154: Prevent naming collisions for series.
Multiple series without anameprop default to"value", potentially overwriting each other’s data keys. Consider generating unique placeholders.- dataKey={series.name || "value"} + dataKey={series.name || `value-${index}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/BarChart/BarChart.tsx(1 hunks)
🔇 Additional comments (6)
packages/ui/src/components/BarChart/BarChart.tsx (6)
1-2: "use client" block is appropriate for Next.js.
No issues with these lines.
5-6: All imported Recharts components appear to be in use.
No concerns regarding these imports.
15-34: Confirm array length consistency and prop usage.
The interface uses parallel arrays forxandy, with no guarantees they match in length. This can create undefined behaviors at runtime. Also note thatheight,barGap,barGroupGap,xAxisLabelInterval, andyAxisLabelIntervalare declared but not visibly applied within the component.
47-56: Verify possible index out-of-bounds.
Usingdata[0].x.map(...)may lead to undefined references if subsequent series have feweryvalues than the first series hasxentries.
73-82: Simple and effective x-axis formatter.
This date vs. string approach is concise and clear.
84-158: Unused props:barGap,barGroupGap,xAxisLabelInterval,yAxisLabelInterval.
These props appear in the interface but aren’t applied in<RechartsBarChart>or related elements. Confirm if this is intended or implement them for flexible chart spacing and label intervals.
Summary by CodeRabbit