[WEB-4740] feat: add propel seperator#7637
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. Warning Rate limit exceeded@JayashTripathy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRemoves Tailwind merge from the classname utility to rely solely on clsx. Adds a new Separator component wrapping @base-ui-components’ SeparatorPrimitive with orientation-based sizing and classes. Introduces Storybook stories for horizontal, vertical, and container-embedded Separator variants. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App/Consumer
participant Sep as Separator (new)
participant Base as SeparatorPrimitive (@base-ui-components)
App->>Sep: Render <Separator orientation=... className=... />
Note over Sep: Compute classes via clsx + orientation sizing
Sep->>Base: Forward props, ref, data-* attrs
Base-->>App: Rendered DOM element (hr/div) sized by orientation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/propel/package.json (2)
16-28: Export the new Separator component (and fix the “seperator” naming) so consumers can import itThere’s no export entry for the new component, and the folder/file are spelled “seperator”. This will make the public import path inconsistent and hard to discover. Ship it as “separator” across the board and expose it via the export map.
Apply this diff to add an export (assuming you also rename the folder to
src/separator/and add anindex.tsre-export):"exports": { "./avatar": "./src/avatar/index.ts", "./charts/*": "./src/charts/*/index.ts", "./dialog": "./src/dialog/index.ts", "./menu": "./src/menu/index.ts", "./table": "./src/table/index.ts", "./tabs": "./src/tabs/index.ts", "./popover": "./src/popover/index.ts", "./command": "./src/command/index.ts", "./combobox": "./src/combobox/index.ts", "./tooltip": "./src/tooltip/index.ts", + "./separator": "./src/separator/index.ts", "./styles/fonts": "./src/styles/fonts/index.css" },And create
packages/propel/src/separator/index.ts:export { Separator } from "./separator"; export type { SeparatorProps } from "./separator";
44-54: Add Storybook essentials so Controls/Docs actually workYou’re configuring Controls in preview.ts, but no addons are registered. Add Essentials to get Actions, Controls, Docs, etc.
"devDependencies": { "@plane/eslint-config": "workspace:*", "@plane/tailwind-config": "workspace:*", "@plane/typescript-config": "workspace:*", "@storybook/react-vite": "^9.1.2", "@types/react": "18.3.1", "@types/react-dom": "18.3.0", + "@storybook/addon-essentials": "^9.1.2", "eslint-plugin-storybook": "^9.1.2", "storybook": "^9.1.2", "typescript": "5.8.3" }packages/propel/src/seperator/seperator.tsx (1)
1-36: Rename misnamed files to “separator”The component and story filenames in
packages/propel/src/separatorare spelled “seperator” but export aSeparatorcomponent. This inconsistency will lead to confusing public import paths and should be corrected now.• Rename under packages/propel/src/separator:
–seperator.tsx→separator.tsx
–seperator.stories.tsx→separator.stories.tsx
• Update imports in the story file:- import { Separator } from "./seperator"; + import { Separator } from "./separator";• Adjust your package’s export map (e.g.
package.jsonexports) or any index files to point to the new filenames.
• Review documentation, README, and tests for lingering references.Additionally, there’s a misspelling in a comment in the editor package:
• In
packages/editor/src/core/extensions/custom-link/helpers/autolink.ts, change// We want to make sure to include the block seperator argument…to
// We want to make sure to include the block separator argument…To find all occurrences before and after renaming, run:
rg -nP '\bseperator\b' -C2
🧹 Nitpick comments (10)
.gitignore (1)
100-101: Ignore pattern looks fine; optionally make it explicit and keep it under the Storybook section.
- The pattern will match
storybook-staticdirectories anywhere. For clarity, consider adding a trailing slash and grouping it with the other Storybook ignores.Apply this small tweak (optional):
-storybook-static +storybook-static/Also consider moving it under the existing
## Storybookheader for maintainability.packages/propel/src/utils/classname.tsx (1)
1-3: Nit: use .ts (no JSX) and consider a short JSDoc.This file doesn’t contain JSX;
.tsis sufficient and avoids extra TSX transforms. Adding a brief JSDoc oncnmakes IDE hovers clearer.turbo.json (1)
28-31: Good addition; consider scoping inputs to improve cache hits and avoid noisy rebuilds.
outputsis correct for Storybook. To improve Turborepo caching and avoid invalidation from unrelated files, consider adding targeted inputs and excluding build artifacts.Example:
"build-storybook": { "dependsOn": ["^build"], - "outputs": ["storybook-static/**"] + "outputs": ["storybook-static/**"], + "inputs": [ + "**/*", + "!**/.next/**", + "!**/dist/**", + "!**/storybook-static/**" + ] },If Storybook is only configured in selected packages, you can also add a convention (e.g., only run where a
storybookscript exists) in the pipeline, but that’s outsideturbo.jsonscope.packages/propel/package.json (1)
12-14: Minor: include storybook-static in the clean script (optional)Handy during local dev to avoid stale static builds.
- "clean": "rm -rf .turbo && rm -rf .next && rm -rf node_modules && rm -rf dist", + "clean": "rm -rf .turbo .next node_modules dist storybook-static",packages/propel/.storybook/preview.ts (1)
4-10: Optional: add default matchers for color/date controlsSmall DX boost; safe default.
const preview: Preview = { parameters: { controls: { - matchers: {}, + matchers: { + color: /(background|color)$/i, + date: /Date$/, + }, }, }, };packages/tailwind-config/global.css (1)
377-383: Global scrollbar hide may surprise story authorsHiding all WebKit scrollbars by default can make debugging long content harder inside Storybook. Consider scoping this (e.g., to app root) or documenting the
.vertical-scrollbar/.horizontal-scrollbarescape hatches.packages/propel/src/seperator/seperator.tsx (1)
5-11: Redundant type:orientationalready exists on the primitive propsSince
SeparatorPropsextends the primitive’s props, redefiningorientationis unnecessary and risks drift if upstream changes. Let the base type own it; document the default in the component JSDoc instead.-interface SeparatorProps extends React.ComponentProps<typeof SeparatorPrimitive> { - /** - * The orientation of the separator - * @default "horizontal" - */ - orientation?: "horizontal" | "vertical"; -} +type SeparatorProps = React.ComponentProps<typeof SeparatorPrimitive>;You can keep the defaulting behavior at the call-site:
-({ orientation = "horizontal", ...props }, ref) => { +({ orientation = "horizontal", ...props }, ref) => { // unchangedpackages/propel/src/seperator/seperator.stories.tsx (3)
4-11: Use “satisfies Meta<...>” for stricter meta typing.This keeps excess properties from slipping in while preserving inference.
-const meta: Meta<typeof Separator> = { +const meta = { title: "Components/Separator", component: Separator, parameters: { layout: "centered", }, tags: ["autodocs"], -}; +} satisfies Meta<typeof Separator>;
16-34: Drive stories via args so variants share the same surface and autodocs stays in sync.Using args instead of hardcoded prop values improves controls/Docs and reduces duplication between Default and Vertical.
export const Default: Story = { - render: () => ( + args: { orientation: "horizontal" }, + render: (args) => ( <div className="w-[300px] space-y-4"> <div>Content Above</div> - <Separator /> + <Separator {...args} /> <div>Content Below</div> </div> ), }; export const Vertical: Story = { - render: () => ( + args: { orientation: "vertical" }, + render: (args) => ( <div className="flex h-[100px] items-center space-x-4"> <div>Left Content</div> - <Separator orientation="vertical" /> + <Separator {...args} /> <div>Right Content</div> </div> ), };
1-1: Optional: Import Storybook types from@storybook/reactinstead of@storybook/react-vitefor future compatibilityOur repo currently only depends on
@storybook/react-vite@^9.1.2and does not have@storybook/reactinstalled. If you’d like to align with the “framework-agnostic” type exports introduced in Storybook 7+ and avoid churn should Storybook split or rename framework packages in the future, you can:
- Add
@storybook/reactto your devDependencies (e.g.yarn add -D @storybook/react@^9.1.2).- Update your import in
packages/propel/src/seperator/seperator.stories.tsxaccordingly.Suggested diff:
-import type { Meta, StoryObj } from "@storybook/react-vite"; +import type { Meta, StoryObj } from "@storybook/react";This change is entirely optional—if your team prefers to standardize on the
react-viteentrypoint today, you can leave it as-is.
📜 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 (11)
.gitignore(1 hunks)packages/propel/.eslintrc.js(1 hunks)packages/propel/.storybook/main.ts(1 hunks)packages/propel/.storybook/preview.ts(1 hunks)packages/propel/package.json(3 hunks)packages/propel/src/seperator/seperator.stories.tsx(1 hunks)packages/propel/src/seperator/seperator.tsx(1 hunks)packages/propel/src/utils/classname.tsx(1 hunks)packages/propel/src/utils/index.ts(1 hunks)packages/tailwind-config/global.css(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/seperator/seperator.stories.tsx (1)
packages/propel/src/seperator/seperator.tsx (1)
Separator(34-34)
⏰ 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). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/propel/src/utils/index.ts (1)
1-1: LGTM — utility barrel export is straightforward.packages/propel/.eslintrc.js (1)
4-4: ESLint: Storybook recommended rules enabled — looks good.The extension is appropriate for the new stories. Assuming
eslint-plugin-storybookis added indevDependencies(as it appears in package.json), no further changes needed.packages/propel/src/seperator/seperator.tsx (1)
22-24: Token usage check: ensurebg-custom-border-200exists in Tailwind themeThe class implies a custom color token. Confirm it’s defined in
@plane/tailwind-configso the separator renders as intended.packages/propel/src/seperator/seperator.stories.tsx (1)
13-15: LGTM: Default export and Story alias are correct.Meta default export and
StoryObjalias pattern is consistent with Storybook recommendations.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new Separator component to the propel package, providing horizontal and vertical divider functionality for UI layouts. The implementation uses Base UI components as the foundation and includes comprehensive Storybook documentation.
- Added a reusable Separator component with horizontal/vertical orientation support
- Updated the classname utility to use only clsx (removing tailwind-merge dependency)
- Created comprehensive Storybook stories demonstrating different separator use cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/propel/src/utils/classname.tsx | Simplified utility to use only clsx, removing tailwind-merge |
| packages/propel/src/seperator/seperator.tsx | New Separator component with orientation prop and Base UI integration |
| packages/propel/src/seperator/seperator.stories.tsx | Storybook stories showcasing separator usage patterns |
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: 2
🧹 Nitpick comments (2)
packages/propel/src/separator/separator.tsx (1)
5-11: Avoid re-declaring orientation if already provided by the primitiveIf
SeparatorPrimitivealready declaresorientation?: "horizontal" | "vertical", redeclaring it inSeparatorPropsis redundant and risks divergence. Consider relying on the inherited prop and moving the default to the component only.packages/propel/src/separator/separator.stories.tsx (1)
16-34: Leverage args/controls to reduce custom rendersExpose
orientationvia controls and use a single template. This simplifies stories and improves Docs/Controls.export default meta; type Story = StoryObj<typeof Separator>; -export const Default: Story = { - render: () => ( - <div className="w-[300px] space-y-4"> - <div>Content Above</div> - <Separator /> - <div>Content Below</div> - </div> - ), -}; +export const Default: Story = { + args: { orientation: "horizontal" }, + render: (args) => ( + <div className="w-[300px] space-y-4"> + <div>Content Above</div> + <Separator {...args} /> + <div>Content Below</div> + </div> + ), +}; -export const Vertical: Story = { - render: () => ( +export const Vertical: Story = { + args: { orientation: "vertical" }, + render: (args) => ( <div className="flex h-[100px] items-center space-x-4"> <div>Left Content</div> - <Separator orientation="vertical" /> + <Separator {...args} /> <div>Right Content</div> </div> ), };Optionally add to meta for better DX:
const meta: Meta<typeof Separator> = { title: "Components/Separator", component: Separator, + argTypes: { + orientation: { control: { type: "inline-radio" }, options: ["horizontal", "vertical"] }, + }, parameters: { layout: "centered" }, tags: ["autodocs"], };
📜 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/src/separator/separator.stories.tsx(1 hunks)packages/propel/src/separator/separator.tsx(1 hunks)packages/propel/src/utils/classname.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/propel/src/utils/classname.tsx
⏰ 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: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/propel/src/separator/separator.tsx (1)
26-29: Typing and ref forwarding look solidGood use of
forwardRef,ElementRef<typeof SeparatorPrimitive>, anddisplayName.packages/propel/src/separator/separator.stories.tsx (1)
36-50: Stories read well and demonstrate real usageThe container example effectively shows typical layout usage. No issues.
Description
This PR adds a separator component to propel package with its respective stories.
Type of Change
Screenshots and Media (if applicable)
Summary by CodeRabbit
New Features
Documentation
Chores