feat: adding baseui components to propel package#7585
feat: adding baseui components to propel package#7585sriramveeraghanta merged 2 commits intopreviewfrom
Conversation
WalkthroughAdds new UI components (Avatar, Dialog, Menu, Tabs) and supporting types/constants under packages/propel; updates package exports and dependencies; adds an ESLint import/order rule and barrel index files. Changes are compile/runtime-neutral aside from new exports and lint configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant App as App UI
participant Dialog as Dialog (Base)
U->>App: Click Dialog.Trigger
App->>Dialog: Open Root/Portal
Dialog-->>App: Render Overlay + Panel
U->>Dialog: Close (outside click / esc)
Dialog-->>App: onOpenChange(false)
sequenceDiagram
participant U as User
participant Menu as Menu
participant Sub as SubMenu
U->>Menu: Open main menu
Menu-->>Sub: provide register/closeAll context
U->>Sub: Hover/Click trigger
Sub-->>Menu: register closer
U->>Sub: Select item
Sub-->>Menu: closeAllSubmenus()
Menu-->>U: Close menus
sequenceDiagram
participant U as User
participant Tabs as Tabs
participant LS as localStorage
U->>Tabs: Select tab (key)
Tabs->>LS: set tab-${storageKey} = key (if enabled)
Tabs-->>U: Render selected Panel
Tabs->>LS: read initial key on mount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
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. ✨ 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 (
|
![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 (30)
packages/propel/.eslintrc.js (1)
27-27: Don't exclude "internal" from pathGroupsExcludedImportTypes; it disables your "@/" pathGroup.**With "internal" excluded, the "@/**" pathGroup won't take effect for internal imports. Keep builtins and the special-case "react" excluded, but allow internals to be ordered by your pathGroup.
Apply this diff:
- pathGroupsExcludedImportTypes: ["builtin", "internal", "react"], + pathGroupsExcludedImportTypes: ["builtin", "react"],packages/propel/package.json (1)
21-22: Use workspace protocol for internal packages instead of "*" to ensure deterministic resolution.For monorepo internals, prefer "workspace:" over "" to avoid accidental registry resolution.
- "@base-ui-components/react": "^1.0.0-beta.2", - "@plane/utils": "*", + "@base-ui-components/react": "^1.0.0-beta.2", + "@plane/utils": "workspace:*",packages/propel/src/dialog/constants.ts (2)
2-4: "TOP" positioning styles center the dialog; adjust to actually top-align.Current classes ("items-center justify-center …") center content. If this enum is used to align at the top edge, use items-start and ensure the container can span the viewport height.
- TOP = "flex items-center justify-center text-center mx-4 my-10 md:my-20", + TOP = "flex items-start justify-center text-center p-4 min-h-full",
11-16: Enum member naming is inconsistent and unclear for 5xl–7xl."VXL", "VIXL", "VIIXL" are cryptic and mix numeral styles. Consider clearer names (e.g., XL5, XL6, XL7 or FIVE_XL, SIX_XL, SEVEN_XL) to improve readability and maintainability.
If you want, I can propose a consistent renaming along with a codemod to update usages across the repo.
packages/propel/src/avatar/avatar.tsx (4)
8-18: "showTooltip" and "name" are not used for any tooltip behavior.Currently only the first letter is used for fallback; no tooltip is rendered. If tooltips are out of scope, either remove the prop or wire a simple native title (as in the previous diff). If you plan to add a Tooltip component, leave a TODO.
I can wire this to your Tooltip component (if available) or remove the prop across the module.
25-58: "spacing" field from getSizeInfo is unused.If it’s reserved for avatar groups, add a comment referencing that pending feature; otherwise, drop it to avoid confusion.
65-74: Border radius is applied three times (wrapper, Root, Fallback).One application at the wrapper is sufficient and reduces duplication and future drift.
Example minimal change (optional):
- <AvatarPrimitive.Root className={cn("h-full w-full", getBorderRadius(shape), className)}> + <AvatarPrimitive.Root className={cn("h-full w-full", className)}> @@ - className={cn(sizeInfo.fontSize, "grid h-full w-full place-items-center", getBorderRadius(shape), className)} + className={cn(sizeInfo.fontSize, "grid h-full w-full place-items-center", className)}Also applies to: 107-110
1-1: Optional: switch to type-only React import and avoid React.FC.Type-only import avoids an unnecessary runtime binding, and using a named props type with a function is idiomatic.
-import React from "react"; +import type { FC } from "react"; @@ -export const Avatar: React.FC<Props> = (props) => { +export const Avatar: FC<Props> = (props) => {Also applies to: 83-83
packages/propel/src/tabs/list.tsx (3)
34-35: Add visible focus styles instead of removing outlinesThe combination of outline-none focus:outline-none removes visible focus indicators, which hurts keyboard accessibility. Prefer focus-visible styles instead of suppressing outlines entirely.
Here’s a minimal tweak that preserves accessibility without picking specific design tokens:
- "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all rounded", + "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 cursor-pointer transition-all rounded",
34-35: Improve spacing between icon and labelWhen both icon and label render, they’ll appear flush. A small gap improves readability.
- "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all rounded", + "flex items-center justify-center gap-1.5 p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 cursor-pointer transition-all rounded",
56-56: Indicator height is fixed; consider reflecting size propIndicator uses fixed h-6 even when size is "sm" or "lg". Dynamically mapping height per size will keep visuals proportional.
If BaseTabs.Indicator accepts className only, you can compute size classes:
- <BaseTabs.Indicator className="absolute left-0 top-[50%] z-[-1] h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out" /> + <BaseTabs.Indicator + className={cn( + "absolute left-0 top-[50%] z-[-1] w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", + { "h-5": size === "sm", "h-6": size === "md", "h-7": size === "lg" } + )} + />packages/propel/src/tabs/tabs.tsx (4)
44-47: Storage key fallback may collide; also reads even when storing is disabled
- When storageKey is absent, the key falls back to tab-${tabs[0]?.key}, which can collide across different views that share the same first tab key.
- With storeInLocalStorage=false, you still read from localStorage (via storedValue), which can surprise consumers expecting no persistence.
Consider only reading when storeInLocalStorage is true, and require or derive a more unique key when persisting.
If you want to avoid reading when disabled, minimally gate the initial tab computation:
- const [activeIndex, setActiveIndex] = useState(() => { - const initialTab = storedValue ?? defaultTab; + const [activeIndex, setActiveIndex] = useState(() => { + const initialTab = storeInLocalStorage ? (storedValue ?? defaultTab) : defaultTab; return tabs.findIndex((tab) => tab.key === initialTab); });
54-58: Avoid unnecessary localStorage writes when tabs array identity changesEffect depends on tabs, which may change identity frequently, causing redundant writes. Narrow dependency to the active tab key.
- }, [activeIndex, setValue, storeInLocalStorage, tabs]); + }, [activeIndex, setValue, storeInLocalStorage, tabs[activeIndex]?.key]);Note: Keep an eye on ESLint’s react-hooks/exhaustive-deps; you may need to justify or refine this depending on how tabs is constructed upstream.
60-65: Prevent selecting disabled tabs defensivelyEven though BaseTabs.Tab is disabled, add a guard to avoid state changes if onValueChange is fired unexpectedly (e.g., programmatically).
- const handleTabChange = (index: number) => { - setActiveIndex(index); - if (!tabs[index].disabled) { - tabs[index].onClick?.(); - } - }; + const handleTabChange = (index: number) => { + if (tabs[index]?.disabled) return; + setActiveIndex(index); + tabs[index].onClick?.(); + };
67-73: Handle empty tabs gracefullyIf tabs is empty, activeIndex will be -1 and BaseTabs.Root value will be invalid. Short-circuit render.
- return ( + if (!tabs?.length) { + return null; + } + return (packages/propel/src/menu/types.ts (3)
12-12: Potential size token typo: "rg"maxHeight includes "rg" alongside "sm" | "md" | "lg". If "rg" was intended to mean "regular", consider aligning with "md" to avoid confusion, or document both. If unintended, drop it.
- maxHeight?: "sm" | "rg" | "md" | "lg"; + maxHeight?: "sm" | "md" | "lg";
24-26: Prefer unknown over any for better type-safetymenuButtonOnClick uses any; consider unknown or a generic for stronger typing. If you need variadic args, keep rest but change the element type.
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - menuButtonOnClick?: (...args: any) => void; + menuButtonOnClick?: (...args: unknown[]) => void;
44-49: Tighten TMenuItemProps onClick arg typeSimilarly, avoid any here. If you need a caller-provided payload, make TMenuItemProps generic.
Option A (simple):
- onClick?: (args?: any) => void; + onClick?: (args?: unknown) => void;Option B (generic):
export type TMenuItemProps<T = void> = { children: React.ReactNode; disabled?: boolean; onClick?: (args: T) => void; className?: string; };packages/propel/src/dialog/index.ts (1)
1-1: Barrel export looks goodSimple and standard. If consumers often need dialog constants (e.g., positions/widths), consider re-exporting them here for convenience.
packages/propel/src/menu/menu.tsx (8)
178-184: Support main menu placementThe
placementprop on Menu is unused. Pass it to the Positioner to control side placement.- <BaseMenu.Positioner - align={"start"} + <BaseMenu.Positioner + side={placement} + align="start"
177-177: Portal target not wired (portalElement prop is ignored)You accept
portalElement?: Element | nullbut do not pass it to the Portal. Base UI libraries usually support acontainerprop on Portal components (naming may vary). Wire it up and verify the prop name per the library.Proposed change (adjust prop name if the library differs):
- <BaseMenu.Portal> + <BaseMenu.Portal container={portalElement ?? undefined}>To confirm the correct prop name on
@base-ui-components/react/menu’s Portal, please check the docs or source.
119-129: Propagate event tomenuButtonOnClickThe optional
menuButtonOnClickis invoked without the event, which may break consumers expecting it.- if (menuButtonOnClick) menuButtonOnClick(); + menuButtonOnClick?.(e);
111-117: Avoid double-callingonMenuClose; defer to onOpenChangeWith the onOpenChange fix,
closeDropdownno longer needs to callonMenuCloseorcloseAllSubmenus. Calling both risks duplicate notifications.- const closeDropdown = React.useCallback(() => { - if (isOpen) { - closeAllSubmenus(); - onMenuClose?.(); - } - setIsOpen(false); - }, [isOpen, closeAllSubmenus, onMenuClose]); + const closeDropdown = React.useCallback(() => { + setIsOpen(false); + }, []);
188-188: Don’t force scrollbars; prefer overflow-y-auto and prevent horizontal scroll
overflow-y-scrollalways shows a scrollbar. Usingoverflow-x-hidden overflow-y-autois typically better UX and matches max-height usage.- "my-1 overflow-y-scroll rounded-md border-[0.5px] border-custom-border-300 bg-custom-background-100 px-2 py-2.5 text-xs shadow-custom-shadow-rg focus:outline-none min-w-[12rem] whitespace-nowrap", + "my-1 overflow-x-hidden overflow-y-auto rounded-md border-[0.5px] border-custom-border-300 bg-custom-background-100 px-2 py-2.5 text-xs shadow-custom-shadow-rg focus:outline-none min-w-[12rem] whitespace-nowrap",
8-13: Remove or implement unused MenuContext wiringYou create
MenuContextand provide it, but nothing consumes it. This is dead code today and can mislead future contributors.If you don’t plan to coordinate submenus via context, remove
MenuContextand the Provider. If you do, add consumers (e.g., submenus registering their closers).Also applies to: 199-199
14-19: SubMenuContext is not provided anywhere
MenuItemreadsSubMenuContext, but no provider exists inSubMenu. Either provide a realcloseSubmenuinSubMenuor remove this context usage.I can help wire a provider if you share how you intend to close submenus (e.g., via BaseMenu API).
Also applies to: 39-42
70-71:placementprop on Menu is currently unusedYou define
placementonTMenuPropsbut ignore it. The earlier suggestion wires it toPositioner.side. If you intended to support both side and alignment, consider exposing both or documenting the behavior.packages/propel/src/dialog/root.tsx (3)
12-22: Overlay/content stacking orderBackdrop and content currently both use
z-30. Depending on DOM order, content may still appear above, but using a higher z-index for content (as suggested above) avoids any ambiguity.
4-4: Import path consistency with MenuMenu imports from
@base-ui-components/react/menu, while Dialog imports from@base-ui-components/react. Consider standardizing import style for consistency and to avoid potential tree-shaking differences.
8-10: Optional: allow DialogPanel to target a custom portal containerIf you foresee rendering the dialog under a specific DOM node (e.g., within an app shell), consider threading a
container?: Elementprop toDialogPortalsimilar to the menu.Please verify the Portal’s container prop name in
@base-ui-components/react(oftencontainerfor libraries with Portal components).Also applies to: 33-61
📜 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)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
packages/propel/.eslintrc.js(1 hunks)packages/propel/package.json(1 hunks)packages/propel/src/avatar/avatar.tsx(1 hunks)packages/propel/src/avatar/index.ts(1 hunks)packages/propel/src/dialog/constants.ts(1 hunks)packages/propel/src/dialog/index.ts(1 hunks)packages/propel/src/dialog/root.tsx(1 hunks)packages/propel/src/menu/index.ts(1 hunks)packages/propel/src/menu/menu.tsx(1 hunks)packages/propel/src/menu/types.ts(1 hunks)packages/propel/src/tabs/list.tsx(1 hunks)packages/propel/src/tabs/tabs.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/propel/src/tabs/tabs.tsx (2)
packages/propel/src/tabs/list.tsx (2)
TabListItem(7-13)TabList(23-58)packages/hooks/src/use-local-storage.tsx (1)
useLocalStorage(24-55)
packages/propel/src/menu/menu.tsx (1)
packages/propel/src/menu/types.ts (3)
TSubMenuProps(35-42)TMenuItemProps(44-49)TMenuProps(3-33)
🔇 Additional comments (7)
packages/propel/src/avatar/index.ts (1)
1-1: Barrel export looks good.Straightforward re-export keeps the public surface tidy.
packages/propel/src/tabs/list.tsx (3)
31-40: Confirm BaseTabs API supports function-valued classNameYou’re passing a function to className on BaseTabs.Tab expecting a
{ selected }arg. Please confirm this is supported by @base-ui-components/react/tabs; some headless tab libs expose selected in render-props or data attributes, not className callbacks.
33-39: Selected state styling vs. actual selection can driftYou derive styles from selectedTab when provided, otherwise from BaseTabs’ selected. This only affects styling; it won’t control BaseTabs’ internal selection. Ensure BaseTabs.Root value/onValueChange keep its selected tab aligned with selectedTab to avoid mismatched indicator/panel vs. header styling.
48-53: Good: per-tab disabled/keys and icon typing are clearUsing a stable key per tab and FC for icon typing is clean and consistent.
packages/propel/src/tabs/tabs.tsx (1)
73-82: Nice composition and APIClean split between header (TabList), actions, and panels. Passing selectedTab to TabList keeps header styling consistent with controlled state.
packages/propel/src/menu/types.ts (1)
31-33: Nice: aria and open state hooks are exposedariaLabel and handleOpenChange in the public API will help with accessibility and state-driven UIs.
packages/propel/src/menu/index.ts (1)
1-2: Barrel exports are consistent and helpfulGood consolidation point for Menu component and its types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/propel/package.json (2)
24-25: Pin prerelease dependency and use file protocol for local utils
- @base-ui-components/react is a beta release; pin it to the exact version to prevent unexpected upgrades.
- Detected yarn@1.22.22 (Yarn Classic) in use, which doesn’t support the
workspace:protocol. To guarantee @plane/utils always links locally, use afile:reference (or upgrade to Yarn 2+ forworkspace:*support).Proposed diff:
- "@base-ui-components/react": "^1.0.0-beta.2", - "@plane/utils": "*", + "@base-ui-components/react": "1.0.0-beta.2", + "@plane/utils": "file:../plane/utils"If you later move to Yarn 2+ (Berry), you can switch back to:
"@plane/utils": "workspace:*"
15-21: Exports Verified – No Deep Imports Detected
- No import statements of the form
from '@plane/propel/(avatar|dialog|menu|tabs)/…'were found across the repo.- Barrel index files exist at:
packages/propel/src/avatar/index.ts
packages/propel/src/dialog/index.ts
packages/propel/src/menu/index.ts
packages/propel/src/tabs/index.tsIf your intent is to lock the API surface and disallow any deep imports, the current
"exports"block is correct and you can merge as-is. If you do want to support deep imports (e.g.@plane/propel/dialog/root), consider adding wildcard exports for each folder:"exports": { "./avatar": "./src/avatar/index.ts", + "./avatar/*": "./src/avatar/*/index.ts", "./dialog": "./src/dialog/index.ts", + "./dialog/*": "./src/dialog/*/index.ts", "./menu": "./src/menu/index.ts", + "./menu/*": "./src/menu/*/index.ts", "./tabs": "./src/tabs/index.ts", + "./tabs/*": "./src/tabs/*/index.ts", "./charts/*": "./src/charts/*/index.ts", "./table": "./src/table/index.ts", "./styles/fonts": "./src/styles/fonts/index.css" },
📜 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 selected for processing (3)
packages/propel/.eslintrc.js(1 hunks)packages/propel/package.json(1 hunks)packages/propel/src/avatar/avatar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/propel/src/avatar/avatar.tsx
- packages/propel/.eslintrc.js
⏰ 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). (3)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
2 participants
Description
Extend propel package with base ui
Type of Change
Summary by CodeRabbit
New Features
Chores