Skip to content

Conversation

@aaryan610
Copy link
Member

@aaryan610 aaryan610 commented Feb 6, 2025

Description

This PR includes-

  1. New chart components-
    • Line chart
    • Area chart
    • Pie chart
  2. Refactored the existing bar chart component to support other formats than stacked.
  3. Refactored existing chart core components, like Tooltip and Ticks.
  4. Refactored existing chart types and constants.

Type of Change

  • Improvement (change that would cause existing functionality to not work as expected)

Summary by CodeRabbit

  • New Features

    • Introduced a suite of new chart components (Area, Bar, Line, and Pie) with enhanced responsiveness and customizable tooltips.
    • Added new styling options to boost visual consistency in chart displays.
  • Refactor

    • Removed legacy chart components and outdated UI elements to streamline the experience.
    • Updated type definitions for improved modularity.
  • Chores

    • Updated configuration and dependency settings to optimize builds, formatting, and overall performance.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 6, 2025

Walkthrough

This 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

File(s) Change Summary
packages/constants/src/chart.ts
packages/constants/src/index.ts
Added new exported constants for chart styling and re-exported the chart module.
packages/types/src/charts.d.ts Updated type definitions: removed TStackItem/TStackChartData, renamed TStackedBarChartProps to TChartProps, and added new types for bar, line, area, and pie charts.
web/core/components/core/charts/stacked-bar-chart/* Removed legacy stacked bar chart components: CustomStackBar, StackedBarChart, and associated CustomTooltip with its type definition.
packages/propel/.prettierignore
packages/propel/.prettierrc
Added new Prettier configuration and ignore patterns for build and temporary directories.
packages/propel/package.json Updated exports (removed globals and components, added ui and charts paths), added lint scripts, removed clsx/tailwind-merge, and added recharts as a dependency.
packages/propel/src/charts/area-chart/*
packages/propel/src/charts/bar-chart/*
packages/propel/src/charts/line-chart/*
packages/propel/src/charts/pie-chart/*
packages/propel/src/charts/tooltip.tsx
packages/propel/src/charts/tree-map/*
Introduced new chart components and their supporting tooltips with re-export patterns for Area, Bar, Line, Pie charts, and Tree Map charts.
packages/propel/src/globals.css
packages/propel/src/ui/button.tsx
Removed obsolete CSS custom properties for theming and the Button component.
packages/tailwind-config/tailwind.config.js Extended Tailwind’s content paths to include propel package source files.
web/next.config.js Added @plane/propel to the transpilePackages configuration.
web/package.json Added dependency for @plane/propel.

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
Loading
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
Loading

Possibly related PRs

Suggested labels

✨feature, 🌟improvement

Suggested reviewers

  • sriramveeraghanta
  • rahulramesha

Poem

I'm a bunny hopping through the code,
New charts and types light up our road,
Constants and components now refined,
With leaps and bounds, our work's aligned,
A joyful nibble on code so sweet—
Hopping along with every feat!
🐰✨

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@aaryan610 aaryan610 added this to the v0.24.0 milestone Feb 6, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7a6b50a and e5cf48a.

📒 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
fi

Length 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?

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:

  1. Add a custom equality function to React.memo to prevent unnecessary re-renders when payload data hasn't changed.
  2. 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:

  1. Add a custom equality function to React.memo to prevent unnecessary re-renders.
  2. Optimize renderCells memoization 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

📥 Commits

Reviewing files that changed from the base of the PR and between e5cf48a and f8f64cc.

📒 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)

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between f8f64cc and 9be999e.

📒 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 of O(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 of O(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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:

  1. Implementing error boundaries to gracefully handle rendering failures
  2. 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-any suggests potential type safety issues. Consider using proper TypeScript types instead of any.


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

📥 Commits

Reviewing files that changed from the base of the PR and between 9be999e and 7447756.

⛔ Files ignored due to path filters (1)
  • yarn.lock is 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 the devDependencies section 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:

  1. Whether @plane/propel provides unique functionality not available in @nivo
  2. If there's a plan to migrate from @nivo to @plane/propel
  3. 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 tsx

Length 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:

  1. The presence or absence of code using both libraries.
  2. 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/graphs and web/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.value might 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.

@sriramveeraghanta sriramveeraghanta merged commit ce57c14 into preview Feb 10, 2025
10 of 14 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Mar 27, 2025
1 task
@aheckmann aheckmann deleted the dev/new-charts branch April 29, 2025 23:07
lifeiscontent pushed a commit that referenced this pull request Aug 18, 2025
* dev: new chart components

* chore: separate out pie chart tooltip

* chore: remove unused any types

* chore: move chart components to propel package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants