[WEB-4727] feat: propel cards#7630
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new Card component and styling helpers under packages/propel/src/card, exposes it via a barrel and package.json export, removes the external @plane/ui dependency from propel, updates chart tooltips to import the local Card, and widens CardProps in packages/ui to extend HTMLDivElement attributes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Tooltip as Tooltip (charts/*/tooltip)
participant Card as Card (propel/src/card)
participant DOM as DOM
User->>Tooltip: Hover or request tooltip
Tooltip->>Card: Render <Card variant, spacing, direction>
Card->>Card: getCardStyle(variant, spacing, direction)
Card->>DOM: Render <div class="..."> with children
Tooltip->>DOM: Insert tooltip content inside Card
note over Tooltip,Card: Tooltip imports Card locally from propel
sequenceDiagram
rect rgba(240,240,255,0.5)
Note over Tooltip,External: Before
participant Tooltip as Tooltip
participant External as @plane/ui Card
Tooltip->>External: import { Card, ECardSpacing } from "@plane/ui"
end
rect rgba(230,255,230,0.4)
Note over Tooltip,Local: After
participant Local as propel/src/card
Tooltip->>Local: import { Card, ECardSpacing } from "../../card"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/propel/package.json (1)
28-40: Remove all lingering “@plane/ui” imports and workspace dependencies
The search uncovered numerous remaining references to the dropped “@plane/ui” package—including inpackages/propel/src/command/command.tsx, your various app workspaces (apps/editor,apps/web,apps/space,apps/admin, etc.), and even within your lockfile (pnpm-lock.yaml). These will break downstream builds now that “@plane/ui” is no longer declared as a dependency.Please update or remove every import and dependency entry for “@plane/ui” (and any of its subpaths) across the repo. Key locations include, but are not limited to:
- packages/propel/src/command/command.tsx (import { cn } from “@plane/ui”)
- packages/editor and packages/propel’s package.jsons still listing “@plane/ui” as a workspace dependency
- apps/editor, apps/web, apps/space, apps/admin workspaces: numerous components importing UI primitives (Button, Tooltip, Loader, etc.) from “@plane/ui”
- pnpm-lock.yaml entries for “@plane/ui”
After removing each import, replace it with the correct package or local subpath (e.g.
@plane/base-ui-components,@plane/propel/card, or your internal UI library), then regenerate the lockfile.
🧹 Nitpick comments (13)
packages/propel/package.json (2)
14-27: Optional: add types conditions in exports for better TS editor hintsIf these subpaths are consumed outside Next.js transpilePackages, adding a “types” condition per subpath improves DX and avoids relying on TS to parse source TS from workspaces. Not required if this package stays private and only used via Next build pipeline.
Example for “./card”:
"exports": { - "./card": "./src/card/index.ts" + "./card": { + "types": "./src/card/index.ts", + "default": "./src/card/index.ts" + } },
14-27: Consider exporting the new Separator (and fix spelling) if it’s part of the public surfaceYou introduced packages/propel/src/seperator/index.tsx. If intended for consumers, add a subpath export and standardize the spelling to “separator” to avoid API churn later.
packages/propel/src/seperator/index.tsx (2)
5-16: Forward the ref for drop-in compatibility with the primitiveWrapping as a simple function drops ref support. Forwarding the ref preserves focus management and integration in parent components.
Patch:
-export function Separator({ className, ...props }: React.ComponentProps<typeof SeparatorPrimitive>) { - return ( - <SeparatorPrimitive +export const Separator = React.forwardRef< + React.ElementRef<typeof SeparatorPrimitive>, + React.ComponentProps<typeof SeparatorPrimitive> +>(({ className, ...props }, ref) => { + return ( + <SeparatorPrimitive + ref={ref} data-slot="separator" className={cn( "bg-border shrink-0 data-[orientation=horizontal]:h-px data-[orientation=horizontal]:w-full data-[orientation=vertical]:h-full data-[orientation=vertical]:w-px", className )} {...props} /> - ); -} + ); +}); +Separator.displayName = "Separator";
1-16: Rename “seperator” directory to “separator” and update related imports/exportsTo prevent the typo from propagating through import paths and package exports, please:
- Rename the folder
packages/propel/src/seperator/→packages/propel/src/separator/- Update all import paths within this package (and any consumers) that reference the old name, for example:
- import { Separator } from "@base-ui-components/react/seperator" + import { Separator } from "@base-ui-components/react/separator"- Add an exports entry for the corrected path in
packages/propel/package.json:"exports": {
- "./separator": "./src/separator/index.ts",
"./avatar": "./src/avatar/index.ts",
"./charts/": "./src/charts//index.ts",
…
}This will ensure your public API and monorepo imports continue to work after renaming. </blockquote></details> <details> <summary>packages/propel/src/card/helper.tsx (3)</summary><blockquote> `22-22`: **Avoid conflicting flex direction classes** DEFAULT_STYLE hardcodes “flex-col”, and directions[ROW] adds “flex-row”, leading to conflicting classes. Tailwind resolves by last-wins, but removing the default “flex-col” is cleaner and future-proof. Apply: ```diff -const DEFAULT_STYLE = - "bg-custom-background-100 rounded-lg border-[0.5px] border-custom-border-200 w-full flex flex-col"; +const DEFAULT_STYLE = + "bg-custom-background-100 rounded-lg border-[0.5px] border-custom-border-200 w-full flex";
13-15: Simplify auxiliary typesThese aliases can just be the enums themselves; reduces duplication without changing behavior.
-export type TCardVariant = ECardVariant.WITHOUT_SHADOW | ECardVariant.WITH_SHADOW; -export type TCardDirection = ECardDirection.ROW | ECardDirection.COLUMN; -export type TCardSpacing = ECardSpacing.SM | ECardSpacing.LG; +export type TCardVariant = ECardVariant; +export type TCardDirection = ECardDirection; +export type TCardSpacing = ECardSpacing;
27-34: Make mappings exhaustive and type-checkedUsing “satisfies Record<…>” ensures all enum keys are covered and values stay string-literals.
-export const spacings = { - [ECardSpacing.SM]: "p-4", - [ECardSpacing.LG]: "p-6", -}; -export const directions = { - [ECardDirection.ROW]: "flex-row space-x-3", - [ECardDirection.COLUMN]: "flex-col space-y-3", -}; +export const spacings = { + [ECardSpacing.SM]: "p-4", + [ECardSpacing.LG]: "p-6", +} as const satisfies Record<TCardSpacing, string>; +export const directions = { + [ECardDirection.ROW]: "flex-row space-x-3", + [ECardDirection.COLUMN]: "flex-col space-y-3", +} as const satisfies Record<TCardDirection, string>;packages/propel/src/charts/tree-map/tooltip.tsx (1)
5-8: Replaceany[]with a precise payload type (fixes ESLint no-explicit-any)The payload shape used below accesses payload[0].payload.{icon,name,value,label}. Define a typed interface to satisfy the linter and improve safety.
-interface TreeMapTooltipProps { - active: boolean | undefined; - payload: any[] | undefined; -} +interface TreeMapDatum { + name?: string; + value: number; + label?: string; + icon?: React.ReactNode; +} +interface TreeMapTooltipItem { + payload: TreeMapDatum; +} +interface TreeMapTooltipProps { + active: boolean | undefined; + payload: TreeMapTooltipItem[] | undefined; +}packages/propel/src/charts/components/tooltip.tsx (1)
4-6: Fix import order to satisfy lint rule (import/order)Place external/aliased imports before relative ones.
-import { Card, ECardSpacing } from "../../card"; - -import { cn } from "@plane/utils"; +import { cn } from "@plane/utils"; +import { Card, ECardSpacing } from "../../card";packages/propel/src/charts/pie-chart/tooltip.tsx (2)
16-19: Add a11y semantics for tooltips.Consider passing
role="tooltip"and an accessible name (e.g.,aria-label={label}) to the container. This improves SR behavior for chart tooltips.Note: This depends on Card accepting native div attributes. See my CardProps comment in
src/card/card.tsx.<Card className="flex flex-col max-h-[40vh] w-[12rem] overflow-y-scroll vertical-scrollbar scrollbar-sm" spacing={ECardSpacing.SM} + role="tooltip" + aria-label={label} >
23-35: Stabilize list keys and provide a color fallback.
key={item?.dataKey}can beundefinedand isn’t guaranteed unique. Safer to fall back to name or index.- If
dotColorisn’t provided, consider falling back toitem?.color(common in Recharts payloads).- {payload?.map((item) => ( - <div key={item?.dataKey} className="flex items-center gap-2 text-xs capitalize"> + {payload?.map((item, idx) => ( + <div key={(item?.dataKey as string) ?? String(item?.name ?? idx)} className="flex items-center gap-2 text-xs capitalize"> <div className="flex items-center gap-2 truncate"> <div className="flex-shrink-0 size-2 rounded-sm" style={{ - backgroundColor: dotColor, + backgroundColor: (item as any)?.color ?? dotColor, }} />packages/propel/src/card/index.ts (1)
1-1: Barrel export is fine; optionally re-export CardProps explicitly.
export *will re-export type-only symbols in TS, but makingCardPropsexplicit can aid discoverability and d.ts generation clarity.export * from "./card"; +export type { CardProps } from "./card";packages/propel/src/card/card.tsx (1)
39-39: Rename displayName to reflect the new package.The current
displayNamereferences the old UI package and may confuse React DevTools users.-Card.displayName = "plane-ui-card"; +Card.displayName = "propel-card";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/propel/package.json(1 hunks)packages/propel/src/card/card.tsx(1 hunks)packages/propel/src/card/helper.tsx(1 hunks)packages/propel/src/card/index.ts(1 hunks)packages/propel/src/charts/components/tooltip.tsx(1 hunks)packages/propel/src/charts/pie-chart/tooltip.tsx(1 hunks)packages/propel/src/charts/tree-map/tooltip.tsx(1 hunks)packages/propel/src/seperator/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/propel/src/card/helper.tsx (1)
packages/propel/src/card/card.tsx (3)
ECardVariant(41-41)ECardDirection(41-41)ECardSpacing(41-41)
packages/propel/src/card/card.tsx (1)
packages/propel/src/card/helper.tsx (4)
TCardVariant(13-13)TCardSpacing(15-15)TCardDirection(14-14)getCardStyle(35-36)
🪛 GitHub Actions: Build and lint web apps
packages/propel/src/charts/components/tooltip.tsx
[warning] 6-6: ESLint: import/order violation: Import order should be adjusted. (import/order)
packages/propel/src/charts/tree-map/tooltip.tsx
[warning] 7-7: ESLint: Unexpected any. Specify a different type. (no-explicit-any)
packages/propel/src/seperator/index.tsx
[error] 17-17: ESLint: Trailing spaces not allowed. (no-trailing-spaces)
🪛 GitHub Check: Build and lint web apps
packages/propel/src/seperator/index.tsx
[failure] 17-17:
Trailing spaces not allowed
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
packages/propel/package.json (1)
25-26: Subpath export for Card looks goodThe new public export (“./card”) is correctly wired to the barrel at ./src/card/index.ts. This unblocks consumers from importing the local Card and enums.
packages/propel/src/charts/tree-map/tooltip.tsx (1)
3-3: Card import migration looks correctSwitching to the local “../../card” module aligns with the dependency removal of @plane/ui.
packages/propel/src/charts/components/tooltip.tsx (1)
4-5: Card import migration looks correctLocalizing the Card usage is consistent with dropping @plane/ui.
packages/propel/src/card/card.tsx (3)
21-37: Component structure and ref forwarding look good.Defaults +
getCardStyle+cnare composed cleanly; the forwardRef wrapper is correct and stable.
2-2: Dependency Declaration VerifiedThe import of
cnfrom@plane/utilsis supported: inpackages/propel/package.json,@plane/utilsis listed underdependencieswith"workspace:*". No further changes are needed.
23-27: Verify defaultvariantimpact on tooltipsAll existing
<Card>usages in Propel explicitly setspacing={ECardSpacing.SM}, so the new defaultspacing = ECardSpacing.LGwon’t affect them. However, none of these overridevariant, meaning they now inherit the new defaultECardVariant.WITH_SHADOW. Please visually confirm that adding the shadow does not introduce unintended elevation or layout changes in the following files:
- packages/propel/src/charts/tree-map/tooltip.tsx
- packages/propel/src/charts/pie-chart/tooltip.tsx
- packages/propel/src/charts/components/tooltip.tsx
JayashTripathy
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds Cards to the propel package and removes its dependency on the UI package to improve modularity. The changes create a standalone Card component within propel to replace external UI dependencies.
- Added new Card component with variant, spacing, and direction configuration options to the propel package
- Updated chart tooltip components to use the local Card implementation instead of the external UI package
- Removed the @plane/ui dependency from propel's package.json
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/card/card.tsx | Extended CardProps interface to include HTML div attributes |
| packages/propel/src/command/command.tsx | Updated import to use @plane/utils instead of @plane/ui for cn utility |
| packages/propel/src/charts/tree-map/tooltip.tsx | Changed Card import from @plane/ui to local card module |
| packages/propel/src/charts/pie-chart/tooltip.tsx | Changed Card import from @plane/ui to local card module |
| packages/propel/src/charts/components/tooltip.tsx | Changed Card import from @plane/ui to local card module |
| packages/propel/src/card/index.ts | Added export barrel file for card module |
| packages/propel/src/card/helper.tsx | Added card configuration enums, types, and styling utilities |
| packages/propel/src/card/card.tsx | Implemented new Card component with variant, spacing, and direction props |
| packages/propel/package.json | Removed @plane/ui dependency and added card export to package exports |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/card/card.tsx (1)
39-39: Action Required: Assign a uniquedisplayNamein the Propel packageBoth the UI and Propel packages currently set their Card component’s
displayNameto"plane-ui-card", which will cause confusion in React DevTools. Update the Propel Card to use a distinct name.• packages/propel/src/card/card.tsx (line 39):
– ChangeCard.displayName = "plane-ui-card"
- to
Card.displayName = "plane-propel-card"Recommended diff:
--- a/packages/propel/src/card/card.tsx +++ b/packages/propel/src/card/card.tsx @@ -37,7 +37,7 @@ }); Card.displayName = "plane-ui-card"; -export { Card, ECardVariant, ECardSpacing, ECardDirection }; +// Renamed for uniqueness in React DevTools +Card.displayName = "plane-propel-card"; +export { Card, ECardVariant, ECardSpacing, ECardDirection };
🧹 Nitpick comments (3)
packages/propel/package.json (1)
26-26: Optional: consider adding a types condition for the new subpathIf consumers (outside the monorepo) ever rely on the new subpath, adding a
typescondition (or ensuring TS path mapping covers it) can improve DX. Existing entries don’t use this, so this is optional for consistency.packages/ui/src/card/card.tsx (2)
13-19: Remove redundant className from CardProps to avoid duplication
classNamealready comes fromReact.HTMLAttributes<HTMLDivElement>. Keeping a duplicate property is harmless but unnecessary.Apply this diff to the interface:
export interface CardProps extends React.HTMLAttributes<HTMLDivElement> { variant?: TCardVariant; spacing?: TCardSpacing; direction?: TCardDirection; - className?: string; children: React.ReactNode; }
23-29: Minor: drop the default empty string for classNameWith the above interface change, letting
classNamebeundefinedis fine and keeps the props shape minimal.cnshould gracefully handleundefined.const { variant = ECardVariant.WITH_SHADOW, direction = ECardDirection.COLUMN, - className = "", + className, spacing = ECardSpacing.LG, children, ...rest } = props;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/propel/package.json(1 hunks)packages/ui/src/card/card.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (4)
packages/propel/package.json (2)
26-26: New subpath export for card is consistent and looks goodAdding
"./card": "./src/card/index.ts"aligns with existing export patterns and will allow@plane/propel/cardimports cleanly.
26-26: No remaining@plane/uiimports in packages/propel/srcAll search results show no imports from
@plane/uiin thepackages/propel/srcdirectory. The only imports ofCardandECardSpacingin chart tooltips are from the new local../../cardmodule, as intended:
- No matches for
from '@plane/ui'inpackages/propel/src- Local imports in chart tooltips (e.g.
import { Card, ECardSpacing } from "../../card";) confirm the new module usageLooks good to merge.
packages/ui/src/card/card.tsx (2)
13-19: Extending HTMLDivElement attributes is a good improvementBroadening
CardPropswithReact.HTMLAttributes<HTMLDivElement>increases flexibility (handlers, data-* attrs, etc.) without affecting runtime.
13-19: No self-closing<Card />usages detected
I searched all.tsxfiles underpackagesfor self-closing<Card />instances and found none. Enforcingchildrenas a required prop should not introduce compile errors in existing call sites.No further changes required.
…etter flexibility
…into feat-propel-cards
Description
Added Cards to propel and removed dependency of UI from propel
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit