-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[WEB-3329] dev: new chart components #6565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request introduces new styling constants for charts and restructures existing chart type definitions. It removes legacy stacked bar chart components from the core web package and adds a comprehensive set of new React chart components (AreaChart, BarChart, LineChart, PieChart, and related tooltips) within the propel package. In addition, configuration files for Prettier, Tailwind, and Next.js have been updated, and obsolete UI components and CSS properties have been removed. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant BC as BarChart Component
participant R as Recharts (CoreBarChart)
participant CB as CustomBar
participant AX as Axes/Tooltip
U->>BC: Render BarChart with props
BC->>BC: Memoize data & configuration (stackKeys, etc.)
BC->>R: Render CoreBarChart with provided props
R->>CB: Render each CustomBar element
R->>AX: Render Axes and Tooltip if enabled
AX->>U: Display chart details
sequenceDiagram
participant U as User
participant PC as PieChart Component
participant R as Recharts (CorePieChart)
participant CT as CustomTooltip (Pie)
U->>PC: Render PieChart with props
PC->>PC: Memoize cell data
PC->>R: Render CorePieChart with data & margins
alt Tooltip enabled
R->>CT: Render CustomPieChartTooltip with payload
end
PC->>U: Display pie chart with interactive tooltips
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Nitpick comments (3)
web/core/components/core/charts/pie-chart/root.tsx (1)
44-67: Consider reusing the CustomTooltip component.The tooltip implementation is similar to the CustomTooltip component. Consider reusing it to maintain consistency and reduce code duplication.
web/core/components/core/charts/line-chart/root.tsx (1)
41-113: Consider extracting common chart configuration into a shared component.The chart configuration (margins, axis styling, tooltip) is duplicated across LineChart, AreaChart, and BarChart components. Consider creating a shared base chart component to reduce code duplication.
I can help create a shared base component that encapsulates common chart configuration. Would you like me to generate the implementation?
web/core/components/core/charts/bar-chart/root.tsx (1)
63-63: Consider removing hardcoded className.The hardcoded "recharts-wrapper" className seems unnecessary as the chart is already wrapped in a div with customizable className.
- className="recharts-wrapper"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/constants/src/chart.ts(1 hunks)packages/constants/src/index.ts(1 hunks)packages/types/src/charts.d.ts(3 hunks)web/core/components/core/charts/area-chart/root.tsx(1 hunks)web/core/components/core/charts/bar-chart/bar.tsx(1 hunks)web/core/components/core/charts/bar-chart/index.ts(1 hunks)web/core/components/core/charts/bar-chart/root.tsx(1 hunks)web/core/components/core/charts/line-chart/index.ts(1 hunks)web/core/components/core/charts/line-chart/root.tsx(1 hunks)web/core/components/core/charts/pie-chart/index.ts(1 hunks)web/core/components/core/charts/pie-chart/root.tsx(1 hunks)web/core/components/core/charts/stacked-bar-chart/bar.tsx(0 hunks)web/core/components/core/charts/stacked-bar-chart/root.tsx(0 hunks)web/core/components/core/charts/stacked-bar-chart/tooltip.tsx(0 hunks)web/core/components/core/charts/tooltip.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- web/core/components/core/charts/stacked-bar-chart/bar.tsx
- web/core/components/core/charts/stacked-bar-chart/root.tsx
- web/core/components/core/charts/stacked-bar-chart/tooltip.tsx
✅ Files skipped from review due to trivial changes (4)
- web/core/components/core/charts/pie-chart/index.ts
- web/core/components/core/charts/line-chart/index.ts
- packages/constants/src/chart.ts
- web/core/components/core/charts/bar-chart/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/core/charts/pie-chart/root.tsx
[error] 15-15: 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)
web/core/components/core/charts/line-chart/root.tsx
[error] 29-29: 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)
web/core/components/core/charts/bar-chart/root.tsx
[error] 31-31: 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)
web/core/components/core/charts/area-chart/root.tsx
[error] 29-29: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
packages/constants/src/index.ts (1)
4-4: LGTM! Verify the chart module exists.The new export is correctly placed in alphabetical order and aligns with the PR objectives of introducing new chart components.
Let's verify that the chart module exists and contains the expected constants:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the existence and contents of the chart module # Check if the chart.ts file exists fd -t f "chart.ts" "packages/constants/src" # If it exists, show its contents if [ $? -eq 0 ]; then cat packages/constants/src/chart.ts fiLength of output: 281
LGTM! The chart module exists and exports the expected constants.
- The file packages/constants/src/chart.ts exists.
- The file exports the constants LABEL_CLASSNAME and AXIS_LINE_CLASSNAME, as intended.
web/core/components/core/charts/tooltip.tsx (1)
1-41: LGTM! Well-structured tooltip component.The implementation is clean, type-safe, and performance-optimized with proper memoization.
web/core/components/core/charts/bar-chart/bar.tsx (1)
8-15: LGTM! Well-implemented percentage calculation.The helper function properly handles zero division case and uses appropriate type constraints.
packages/types/src/charts.d.ts (1)
1-119: LGTM! Well-designed type system for charts.The type definitions are comprehensive, properly constrained, and promote code reuse through composition.
web/core/components/core/charts/area-chart/root.tsx (1)
36-44: Consider adding error boundaries for chart rendering.The Area component might fail to render if the data is malformed. Consider wrapping the chart in an error boundary to gracefully handle rendering failures.
Would you like me to generate an implementation of a chart-specific error boundary component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (4)
web/core/components/core/charts/pie-chart/tooltip.tsx (2)
13-31: Consider optimizing performance.The component is memoized but could benefit from additional optimizations:
- Add a custom equality function to
React.memoto prevent unnecessary re-renders when payload data hasn't changed.- Memoize the payload map operation.
Apply this diff to optimize performance:
-export const CustomPieChartTooltip = React.memo((props: Props) => { +export const CustomPieChartTooltip = React.memo((props: Props) => { const { dotClassName, label, payload } = props; + const renderedPayload = useMemo( + () => + payload?.map((item) => ( + <div key={item?.dataKey} className="flex items-center gap-2 text-xs capitalize"> + <div className={cn("size-2 rounded-full", dotClassName)} /> + <span className="text-custom-text-300">{item?.name}:</span> + <span className="font-medium text-custom-text-200">{item?.value}</span> + </div> + )), + [payload, dotClassName] + ); + return ( <Card className="flex flex-col" spacing={ECardSpacing.SM}> <p className="text-xs text-custom-text-100 font-medium border-b border-custom-border-200 pb-2 capitalize"> {label} </p> - {payload?.map((item) => ( - <div key={item?.dataKey} className="flex items-center gap-2 text-xs capitalize"> - <div className={cn("size-2 rounded-full", dotClassName)} /> - <span className="text-custom-text-300">{item?.name}:</span> - <span className="font-medium text-custom-text-200">{item?.value}</span> - </div> - ))} + {renderedPayload} </Card> ); -}); +}, (prevProps, nextProps) => { + return ( + prevProps.label === nextProps.label && + prevProps.dotClassName === nextProps.dotClassName && + JSON.stringify(prevProps.payload) === JSON.stringify(nextProps.payload) + ); +});
16-29: Add ARIA attributes for better accessibility.The tooltip should have proper ARIA attributes to improve accessibility for screen readers.
Apply this diff to enhance accessibility:
return ( - <Card className="flex flex-col" spacing={ECardSpacing.SM}> + <Card + role="tooltip" + aria-label={`${label} chart data`} + className="flex flex-col" + spacing={ECardSpacing.SM} + > <p className="text-xs text-custom-text-100 font-medium border-b border-custom-border-200 pb-2 capitalize"> {label} </p> {payload?.map((item) => ( - <div key={item?.dataKey} className="flex items-center gap-2 text-xs capitalize"> + <div + key={item?.dataKey} + className="flex items-center gap-2 text-xs capitalize" + aria-label={`${item?.name}: ${item?.value}`} + > <div className={cn("size-2 rounded-full", dotClassName)} /> <span className="text-custom-text-300">{item?.name}:</span> <span className="font-medium text-custom-text-200">{item?.value}</span>web/core/components/core/charts/pie-chart/root.tsx (2)
10-50: Consider optimizing performance.The component is memoized but could benefit from additional optimizations:
- Add a custom equality function to
React.memoto prevent unnecessary re-renders.- Optimize
renderCellsmemoization by using a stable key.Apply this diff to optimize performance:
-export const PieChart = React.memo(<K extends string, T extends string>(props: TPieChartProps<K, T>) => { +export const PieChart = React.memo(<K extends string, T extends string>(props: TPieChartProps<K, T>) => { const { data, dataKey, cells, className = "w-full h-96", innerRadius, outerRadius, showTooltip = true } = props; const renderCells = useMemo( - () => cells.map((cell) => <Cell key={cell.key} className={cell.className} style={cell.style} />), - [cells] + () => + cells.map((cell) => ( + <Cell + key={`${cell.key}-${cell.className}-${JSON.stringify(cell.style)}`} + className={cell.className} + style={cell.style} + /> + )), + [cells] ); // ... rest of the component -}); +}, (prevProps, nextProps) => { + return ( + prevProps.dataKey === nextProps.dataKey && + prevProps.className === nextProps.className && + prevProps.innerRadius === nextProps.innerRadius && + prevProps.outerRadius === nextProps.outerRadius && + prevProps.showTooltip === nextProps.showTooltip && + JSON.stringify(prevProps.data) === JSON.stringify(nextProps.data) && + JSON.stringify(prevProps.cells) === JSON.stringify(nextProps.cells) + ); +});
18-48: Add ARIA attributes for better accessibility.The chart should have proper ARIA attributes to improve accessibility for screen readers.
Apply this diff to enhance accessibility:
return ( - <div className={className}> + <div + role="figure" + aria-label={`${dataKey} pie chart`} + className={className} + > <ResponsiveContainer width="100%" height="100%"> <CorePieChart width={500} height={300} data={data} margin={{ top: 5, right: 30, left: 20, bottom: 5, }} > - <Pie data={data} dataKey={dataKey} cx="50%" cy="50%" innerRadius={innerRadius} outerRadius={outerRadius}> + <Pie + data={data} + dataKey={dataKey} + cx="50%" + cy="50%" + innerRadius={innerRadius} + outerRadius={outerRadius} + role="presentation" + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/core/components/core/charts/pie-chart/root.tsx(1 hunks)web/core/components/core/charts/pie-chart/tooltip.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/core/components/core/charts/bar-chart/root.tsx (1)
43-45: Replace 'any' type for shape props with a typed approach.Using (shapeProps: any) prevents leveraging TypeScript’s type system. You can import the corresponding Recharts types (e.g., RectangleProps) or define a custom interface for shapeProps to improve clarity and safety.
-import { Bar } from "recharts"; +import { Bar, RectangleProps } from "recharts"; shape={(shapeProps: any) => ( <CustomBar {...shapeProps} stackKeys={stackKeys} ... /> )} + // Proposed approach: +shape={(shapeProps: RectangleProps) => ( + <CustomBar + {...shapeProps} + stackKeys={stackKeys} + ... + /> +)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/core/components/core/charts/bar-chart/bar.tsx(1 hunks)web/core/components/core/charts/bar-chart/root.tsx(1 hunks)web/core/components/core/charts/pie-chart/root.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/core/charts/pie-chart/root.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/core/charts/bar-chart/root.tsx
[error] 31-31: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
web/core/components/core/charts/bar-chart/root.tsx (2)
30-33: Use direct object assignment instead of spread in reduce.This usage of the spread operator on every iteration can degrade performance (O(n^2)). Consider mutating the accumulator directly or using Object.assign for efficiency.
- const stackDotClassNames = useMemo( - () => bars.reduce((acc, bar) => ({ ...acc, [bar.key]: bar.dotClassName }), {}), - [bars] - ); + const stackDotClassNames = useMemo( + () => + bars.reduce((acc, bar) => { + acc[bar.key] = bar.dotClassName; + return acc; + }, {} as Record<string, string | undefined>), + [bars] + );🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: 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)
1-126: All other parts of this file look great.The overall structure and logic are clear and maintainable.
🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: 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)
web/core/components/core/charts/bar-chart/bar.tsx (3)
1-1: Remove the eslint-disable comment and fix any type usage.Instead of disabling eslint, define explicit types that align with Recharts or the data structures used.
-/* eslint-disable @typescript-eslint/no-explicit-any */
20-21: Replace 'any' for props with a typed interface.Defining an interface or using generics helps ensure type safety and clarity.
-export const CustomBar = React.memo((props: any) => { +interface CustomBarProps<K extends string, T extends string> { + fill: string; + x: number; + y: number; + width: number; + height: number; + dataKey: T; + stackKeys: T[]; + payload: TChartData<K, T>; + textClassName?: string; + showPercentage?: boolean; +} + +export const CustomBar = React.memo(<K extends string, T extends string>(props: CustomBarProps<K, T>) => {
1-71: All other parts of this file look good.Your approach with the rounded corners and conditional percentage display is concise and well-structured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (6)
packages/propel/src/charts/pie-chart/tooltip.tsx (1)
13-31: Consider adding ARIA labels for better accessibility.The implementation looks good, but consider adding ARIA labels to convey color information for screen readers.
Apply this diff to improve accessibility:
- <div key={item?.dataKey} className="flex items-center gap-2 text-xs capitalize"> + <div + key={item?.dataKey} + className="flex items-center gap-2 text-xs capitalize" + role="listitem" + aria-label={`${item?.name}: ${item?.value}`} + >packages/propel/src/charts/pie-chart/root.tsx (2)
18-50: Consider making dimensions and margins configurable via props.The implementation looks good, but consider making the chart dimensions and margins configurable for better flexibility.
Update the props type to include margin configuration and apply this diff:
- margin={{ - top: 5, - right: 30, - left: 20, - bottom: 5, - }} + margin={props.margin ?? { + top: 5, + right: 30, + left: 20, + bottom: 5, + }}Also, consider making the width and height configurable:
- width={500} - height={300} + width={props.width ?? 500} + height={props.height ?? 300}
10-50: Consider adding error boundaries and loading states.To improve reliability and user experience, consider:
- Implementing error boundaries to gracefully handle rendering failures
- Adding loading states for data fetching scenarios
Example implementation:
import { ErrorBoundary } from '@plane/ui'; export const PieChart = React.memo(<K extends string, T extends string>(props: TPieChartProps<K, T>) => { const { data, isLoading, ...rest } = props; if (isLoading) { return <div className="animate-pulse">Loading chart...</div>; } return ( <ErrorBoundary fallback={<div>Error loading chart</div>}> {/* existing implementation */} </ErrorBoundary> ); });packages/propel/src/charts/area-chart/root.tsx (1)
33-47: Consider extracting renderAreas logic to a separate component.The area rendering logic is similar to the line rendering in LineChart. Consider extracting this to a shared component for better maintainability.
Create a new shared component:
// packages/propel/src/charts/shared/chart-elements.tsx interface RenderElementsProps<T extends string> { elements: Array<{ key: T; className?: string; stackId?: string; }>; ElementComponent: typeof Area | typeof Line; } export const RenderElements = React.memo(<T extends string>({ elements, ElementComponent }: RenderElementsProps<T>) => ( <> {elements.map((element) => ( <ElementComponent key={element.key} type="monotone" dataKey={element.key} stackId={element.stackId} className={element.className} stroke="inherit" fill="inherit" /> ))} </> ));packages/propel/src/charts/bar-chart/root.tsx (2)
1-1: Consider removing the ESLint disable comment.The ESLint disable comment for
@typescript-eslint/no-explicit-anysuggests potential type safety issues. Consider using proper TypeScript types instead ofany.
107-107: Consider extracting the tooltip cursor style to a constant.The tooltip cursor style could be reused across different chart components.
Apply this diff to extract the style:
+const TOOLTIP_CURSOR_STYLE = "text-custom-background-90/80 cursor-pointer"; // ... - cursor={{ fill: "currentColor", className: "text-custom-background-90/80 cursor-pointer" }} + cursor={{ fill: "currentColor", className: TOOLTIP_CURSOR_STYLE }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
packages/propel/.prettierignore(1 hunks)packages/propel/.prettierrc(1 hunks)packages/propel/package.json(1 hunks)packages/propel/src/charts/area-chart/root.tsx(1 hunks)packages/propel/src/charts/bar-chart/bar.tsx(1 hunks)packages/propel/src/charts/bar-chart/root.tsx(1 hunks)packages/propel/src/charts/line-chart/index.ts(1 hunks)packages/propel/src/charts/line-chart/root.tsx(1 hunks)packages/propel/src/charts/pie-chart/index.ts(1 hunks)packages/propel/src/charts/pie-chart/root.tsx(1 hunks)packages/propel/src/charts/pie-chart/tooltip.tsx(1 hunks)packages/propel/src/charts/tooltip.tsx(1 hunks)packages/propel/src/charts/tree-map/index.ts(1 hunks)packages/propel/src/globals.css(0 hunks)packages/propel/src/ui/button.tsx(0 hunks)packages/tailwind-config/tailwind.config.js(1 hunks)web/next.config.js(1 hunks)web/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/propel/src/globals.css
- packages/propel/src/ui/button.tsx
✅ Files skipped from review due to trivial changes (5)
- packages/propel/src/charts/tree-map/index.ts
- packages/propel/.prettierignore
- packages/propel/src/charts/pie-chart/index.ts
- packages/propel/src/charts/line-chart/index.ts
- packages/propel/.prettierrc
🧰 Additional context used
🪛 Biome (1.9.4)
packages/propel/src/charts/line-chart/root.tsx
[error] 29-29: 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/propel/src/charts/area-chart/root.tsx
[error] 29-29: 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/propel/src/charts/bar-chart/root.tsx
[error] 31-31: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
packages/propel/package.json (4)
5-8: New Lint Scripts Added
The addition of the"lint"and"lint:errors"scripts to run ESLint on TypeScript files enhances the code quality assurance workflow. This change aligns well with the overall improvement objectives.
9-12: Updated Exports Mapping
The exports section has been updated to expose UI and chart components via"./ui/*"and"./charts/*". This change reflects the refactor efforts and new chart components structure. Ensure that these export paths correctly map to the intended file locations in the project structure.
19-21: Recharts Dependency Inclusion
The addition of"recharts": "^2.15.1"in the dependencies is appropriate given the introduction of new chart components that leverage Recharts. Verify that this version is compatible with the rest of the project and does not introduce any conflicts.
22-28: DevDependencies Restructuring for Clarity
The restructuring of thedevDependenciessection improves readability by grouping and ordering the dependencies cleanly. Although the actual dependency versions remain unchanged, this organization helps maintain consistency across the file.packages/propel/src/charts/pie-chart/tooltip.tsx (1)
1-11: LGTM! Well-structured imports and type definitions.The imports are properly organized and the Props type is well-defined with appropriate type safety using recharts types.
packages/propel/src/charts/pie-chart/root.tsx (2)
1-12: LGTM! Well-structured component with proper TypeScript generics.Good use of TypeScript generics and proper default values for props.
13-16: LGTM! Proper performance optimization with useMemo.The cell rendering logic is well-optimized using useMemo with the correct dependency array.
web/next.config.js (1)
21-21: LGTM! Necessary configuration for the new chart components.The addition of "@plane/propel" to transpilePackages is correct and aligns with the introduction of new chart components.
web/package.json (1)
24-30: Verify overlap with existing chart libraries.The project already uses @nivo chart libraries. Please verify:
- Whether @plane/propel provides unique functionality not available in @nivo
- If there's a plan to migrate from @nivo to @plane/propel
- Whether maintaining multiple chart libraries is intentional
Let me help verify the usage of chart libraries in the codebase:
Also applies to: 35-35
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check usage of chart libraries in the codebase echo "Checking @nivo usage..." rg -l '@nivo' --type ts --type tsx echo -e "\nChecking @plane/propel chart usage..." rg -l '@plane/propel.*Chart' --type ts --type tsxLength of output: 290
I'll adjust our search commands to use explicit glob patterns (i.e. ".ts" and ".tsx") instead of the unsupported
--type tsx. This should accurately list files that reference these libraries. Please run the following updated script:This will help us verify:
- The presence or absence of code using both libraries.
- Whether @plane/propel is integrated uniquely compared to @nivo.
Once we see the output from this script, we can further determine if there’s any migration plan or intentional coexistence.
🏁 Script executed:
#!/bin/bash echo "Searching for @nivo usages in TS/TSX files..." rg -l '@nivo' -g "*.ts" -g "*.tsx" echo -e "\nSearching for @plane/propel usages in TS/TSX files..." rg -l '@plane/propel' -g "*.ts" -g "*.tsx"Length of output: 1037
Action Required: Verify Chart Library Usage Consistency
- Usage of @nivo libraries is confirmed in several TS/TSX files (e.g., components under
web/core/components/ui/graphsandweb/core/components/analytics).- No occurrences of @plane/propel were found in the codebase.
- There is no indication within the code of an ongoing or planned migration from @nivo to @plane/propel, nor evidence that maintaining multiple chart libraries is currently intentional.
packages/propel/src/charts/tooltip.tsx (1)
27-37: Add type checking for payload item values.The component should handle cases where
item.valuemight be undefined or of unexpected type.Consider adding type guards:
<div key={item?.dataKey} className="flex items-center gap-2 text-xs capitalize"> {itemDotClassNames[item?.dataKey] && ( <div className={cn("size-2 rounded-full", itemDotClassNames[item?.dataKey])} /> )} <span className="text-custom-text-300">{item?.name}:</span> - <span className="font-medium text-custom-text-200">{item?.value}</span> + <span className="font-medium text-custom-text-200"> + {typeof item?.value === 'number' ? item.value.toLocaleString() : item?.value} + </span> </div>packages/tailwind-config/tailwind.config.js (1)
22-22: LGTM!The addition of the propel package path to the content configuration is necessary for the new chart components to work correctly with Tailwind CSS.
* dev: new chart components * chore: separate out pie chart tooltip * chore: remove unused any types * chore: move chart components to propel package
Description
This PR includes-
TooltipandTicks.Type of Change
Summary by CodeRabbit
New Features
Refactor
Chores