feat: add Popover, Skeleton, and ScrollArea components with Storybook stories#50
feat: add Popover, Skeleton, and ScrollArea components with Storybook stories#50
Conversation
… stories - Introduced new `Popover`, `Skeleton`, and `ScrollArea` components in the UI library. - Added Storybook stories for each component (`Popover.stories.tsx`, `Skeleton.stories.tsx`, `ScrollArea.stories.tsx`) showcasing their usage and variations. - Exported new components in the `ui/index.ts` for external use.
📝 WalkthroughWalkthroughAdds three new UI components (Popover, ScrollArea, Skeleton), Storybook stories for each, and re-exports them from the UI barrel and main index. All changes are additive with no changes to existing runtime behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Trigger as PopoverTrigger
participant Portal as PopoverPortal
participant Content as PopoverContent
User->>Trigger: click
Trigger->>Portal: open portal
Portal->>Content: mount content
Content-->>User: render panel (Title, Description, Inputs)
User->>Content: interact / close
Content->>Portal: unmount/close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/ui/scroll-area.tsx`:
- Around line 6-24: The ScrollBar child is being rendered inside
ScrollAreaPrimitive.Content instead of as a sibling of Viewport, so update the
ScrollArea component to extract any ScrollBar children from the children prop
and render them alongside the existing <ScrollBar /> at the Root level (outside
ScrollAreaPrimitive.Content); specifically, in the ScrollArea function filter
React.Children.toArray(children) for elements with type === ScrollBar (or a
distinguishing prop), keep non-scrollbar children to render inside
ScrollAreaPrimitive.Content and render the extracted ScrollBar elements after
the Viewport (alongside the hardcoded <ScrollBar />) so horizontal/additional
scrollbars from the Horizontal and Both stories function correctly.
In `@src/components/ui/ScrollArea.stories.tsx`:
- Around line 40-69: The Horizontal and Both stories currently render
<ScrollBar> as children inside <ScrollArea> (ending up in
ScrollAreaPrimitive.Content) so they won't show functional scrollbars; update
the stories (Horizontal, Both) to match the revised ScrollArea API by moving or
mounting the <ScrollBar orientation="..."> at the ScrollArea root/slot required
by the new API (or use the new prop/slot/component the ScrollArea exposes for
scrollbars) so that the scrollbar is attached at the Root level rather than
inside Content; ensure you reference ScrollArea and ScrollBar symbols when
updating the render functions so orientation ("horizontal"/"vertical") is
applied per the new API.
🧹 Nitpick comments (4)
src/components/ui/scroll-area.tsx (1)
1-2: UnusedReactimport.With the modern JSX transform,
import * as React from "react"is unnecessary here since no React APIs (e.g.,React.Fragment,React.useState) are used directly.src/components/ui/Skeleton.stories.tsx (1)
33-58: Consider reducing duplication in the List story.The three list items are identical. A
.map()would reduce the boilerplate, consistent with how theDefaultstory inScrollArea.stories.tsxgenerates items.Suggested refactor
export const List: Story = { render: () => ( <div className="space-y-4 w-[300px]"> - <div className="flex items-center space-x-4"> - <Skeleton className="h-10 w-10 rounded-full" /> - <div className="space-y-2 flex-1"> - <Skeleton className="h-4 w-full" /> - <Skeleton className="h-3 w-3/4" /> - </div> - </div> - <div className="flex items-center space-x-4"> - <Skeleton className="h-10 w-10 rounded-full" /> - <div className="space-y-2 flex-1"> - <Skeleton className="h-4 w-full" /> - <Skeleton className="h-3 w-3/4" /> - </div> - </div> - <div className="flex items-center space-x-4"> - <Skeleton className="h-10 w-10 rounded-full" /> - <div className="space-y-2 flex-1"> - <Skeleton className="h-4 w-full" /> - <Skeleton className="h-3 w-3/4" /> - </div> - </div> + {Array.from({ length: 3 }).map((_, i) => ( + <div key={i} className="flex items-center space-x-4"> + <Skeleton className="h-10 w-10 rounded-full" /> + <div className="space-y-2 flex-1"> + <Skeleton className="h-4 w-full" /> + <Skeleton className="h-3 w-3/4" /> + </div> + </div> + ))} </div> ), };src/components/ui/popover.tsx (2)
15-17: ExportedPopoverPortalmay confuse consumers sincePopoverContentalready renders its own Portal.
PopoverContent(line 33) internally wraps everything inPopoverPrimitive.Portal, so if a consumer also wrapsPopoverContentinPopoverPortal, the content gets double-portaled. Consider either removing this export or documenting that it's only needed for advanced/custom composition outside ofPopoverContent.
54-68: Simple pass-through wrappers look fine.Minor nit: the
{ ...props }destructuring pattern (e.g.,{ ...props }: Type) creates an unnecessary shallow copy. You could use(props: Type)directly. This applies to all the simple wrappers in this file.
| function ScrollArea({ | ||
| className, | ||
| children, | ||
| ...props | ||
| }: ScrollAreaPrimitive.Root.Props) { | ||
| return ( | ||
| <ScrollAreaPrimitive.Root | ||
| data-slot="scroll-area" | ||
| className={cn("relative overflow-hidden", className)} | ||
| {...props} | ||
| > | ||
| <ScrollAreaPrimitive.Viewport className="h-full w-full rounded-[inherit] outline-none"> | ||
| <ScrollAreaPrimitive.Content>{children}</ScrollAreaPrimitive.Content> | ||
| </ScrollAreaPrimitive.Viewport> | ||
| <ScrollBar /> | ||
| <ScrollAreaPrimitive.Corner /> | ||
| </ScrollAreaPrimitive.Root> | ||
| ); | ||
| } |
There was a problem hiding this comment.
ScrollBar children passed to ScrollArea are rendered inside Content, not at Root level.
Children are placed inside ScrollAreaPrimitive.Content (Line 18), so any <ScrollBar> added as a child (as done in the Horizontal and Both stories) will be nested inside the scrollable content area rather than being a sibling of Viewport at the Root level. This means horizontal/additional scrollbars won't function correctly in those stories.
Consider either:
- Filtering
ScrollBarinstances out ofchildrenand rendering them alongside the hardcoded one, or - Accepting a prop like
orientationto control which scrollbars to render internally.
Option 2: Add orientation prop
+type ScrollAreaProps = ScrollAreaPrimitive.Root.Props & {
+ orientation?: "vertical" | "horizontal" | "both";
+};
+
function ScrollArea({
className,
children,
+ orientation = "vertical",
...props
-}: ScrollAreaPrimitive.Root.Props) {
+}: ScrollAreaProps) {
return (
<ScrollAreaPrimitive.Root
data-slot="scroll-area"
className={cn("relative overflow-hidden", className)}
{...props}
>
<ScrollAreaPrimitive.Viewport className="h-full w-full rounded-[inherit] outline-none">
<ScrollAreaPrimitive.Content>{children}</ScrollAreaPrimitive.Content>
</ScrollAreaPrimitive.Viewport>
- <ScrollBar />
+ {(orientation === "vertical" || orientation === "both") && (
+ <ScrollBar orientation="vertical" />
+ )}
+ {(orientation === "horizontal" || orientation === "both") && (
+ <ScrollBar orientation="horizontal" />
+ )}
<ScrollAreaPrimitive.Corner />
</ScrollAreaPrimitive.Root>
);
}🤖 Prompt for AI Agents
In `@src/components/ui/scroll-area.tsx` around lines 6 - 24, The ScrollBar child
is being rendered inside ScrollAreaPrimitive.Content instead of as a sibling of
Viewport, so update the ScrollArea component to extract any ScrollBar children
from the children prop and render them alongside the existing <ScrollBar /> at
the Root level (outside ScrollAreaPrimitive.Content); specifically, in the
ScrollArea function filter React.Children.toArray(children) for elements with
type === ScrollBar (or a distinguishing prop), keep non-scrollbar children to
render inside ScrollAreaPrimitive.Content and render the extracted ScrollBar
elements after the Viewport (alongside the hardcoded <ScrollBar />) so
horizontal/additional scrollbars from the Horizontal and Both stories function
correctly.
| export const Horizontal: Story = { | ||
| render: () => ( | ||
| <ScrollArea className="w-96 whitespace-nowrap rounded-md border"> | ||
| <div className="flex w-max space-x-4 p-4"> | ||
| {Array.from({ length: 20 }).map((_, i) => ( | ||
| <div | ||
| key={i} | ||
| className="bg-muted flex h-40 w-32 items-center justify-center rounded-md" | ||
| > | ||
| Item {i + 1} | ||
| </div> | ||
| ))} | ||
| </div> | ||
| <ScrollBar orientation="horizontal" /> | ||
| </ScrollArea> | ||
| ), | ||
| }; | ||
|
|
||
| export const Both: Story = { | ||
| render: () => ( | ||
| <ScrollArea className="h-72 w-96 rounded-md border"> | ||
| <div className="grid h-[500px] w-[600px] place-items-center bg-muted/20"> | ||
| <div className="text-sm font-medium"> | ||
| Large content area (600x500px) | ||
| </div> | ||
| </div> | ||
| <ScrollBar orientation="horizontal" /> | ||
| <ScrollBar orientation="vertical" /> | ||
| </ScrollArea> | ||
| ), |
There was a problem hiding this comment.
Horizontal and Both stories won't render functional scrollbars.
As noted in the scroll-area.tsx review, the <ScrollBar> components placed as children here end up inside ScrollAreaPrimitive.Content rather than at the Root level. These stories will need to be updated once the ScrollArea component API is revised to support configurable orientations.
🤖 Prompt for AI Agents
In `@src/components/ui/ScrollArea.stories.tsx` around lines 40 - 69, The
Horizontal and Both stories currently render <ScrollBar> as children inside
<ScrollArea> (ending up in ScrollAreaPrimitive.Content) so they won't show
functional scrollbars; update the stories (Horizontal, Both) to match the
revised ScrollArea API by moving or mounting the <ScrollBar orientation="...">
at the ScrollArea root/slot required by the new API (or use the new
prop/slot/component the ScrollArea exposes for scrollbars) so that the scrollbar
is attached at the Root level rather than inside Content; ensure you reference
ScrollArea and ScrollBar symbols when updating the render functions so
orientation ("horizontal"/"vertical") is applied per the new API.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/ui/Popover.stories.tsx`:
- Around line 21-23: The story nests a Button inside PopoverTrigger which causes
invalid nested <button> elements; update the PopoverTrigger usage (symbol:
PopoverTrigger) to render a non-button wrapper or use its render prop so it
doesn't output a <button> around the Button component (symbol: Button). Replace
the current children usage with PopoverTrigger's render prop (or pass Button
directly via render) so the trigger renders a div/span/fragment or returns the
Button element itself, ensuring only a single interactive element is output and
preserving accessibility and click behavior.
🧹 Nitpick comments (1)
src/components/ui/popover.tsx (1)
15-17:PopoverPortalis dead code — defined but neither exported nor used internally.
PopoverContentcreates its ownPopoverPrimitive.Portal(line 33), so this standalone wrapper serves no purpose. Remove it to avoid confusion.🧹 Proposed fix
-function PopoverPortal({ ...props }: PopoverPrimitive.Portal.Props) { - return <PopoverPrimitive.Portal data-slot="popover-portal" {...props} />; -} -
| <PopoverTrigger> | ||
| <Button variant="outline">Open Popover</Button> | ||
| </PopoverTrigger> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
base-ui react Popover Trigger component API documentation
💡 Result:
Base UI’s React Popover.Trigger API docs are on the Popover component page, under API reference → Trigger. [1]
<Popover.Trigger /> (renders a <button> by default) props: [1]
handle?: Popover.Handle<Payload>— links a (possibly detached) trigger to a popover handlenativeButton?: boolean(defaulttrue) — when usingrender, keep native<button>semantics; setfalseif your rendered element isn’t a buttonpayload?: Payload— value passed toPopover.Rootwhen opened (useful with multiple triggers)openOnHover?: boolean(defaultfalse) — also open on hoverdelay?: number(default300) — hover-open delay (ms); requiresopenOnHovercloseDelay?: number(default0) — hover-close delay (ms); requiresopenOnHoverid?: string— trigger ID (used for controlled mode viaPopover.Root triggerId)className?: string | (state) => stringstyle?: CSSProperties | (state) => CSSPropertiesrender?: ReactElement | (props, state) => ReactElement— replace/compose the rendered element
State/data attributes noted in the docs: data-popup-open, data-pressed. [1]
Source
- [1] Base UI Popover docs (API reference → Trigger): (base-ui.com)
Citations:
🏁 Script executed:
# First, let's find and examine the Popover component implementation
find . -type f -name "*opover*" | head -20Repository: getdokan/plugin-ui
Length of output: 133
🏁 Script executed:
# Let's also check the Button component
find . -type f -name "*utton*" | head -20Repository: getdokan/plugin-ui
Length of output: 278
🏁 Script executed:
# Read the Popover component implementation
cat -n ./src/components/ui/popover.tsxRepository: getdokan/plugin-ui
Length of output: 3527
🏁 Script executed:
# Read the Button component implementation
cat -n ./src/components/ui/button.tsxRepository: getdokan/plugin-ui
Length of output: 4936
🏁 Script executed:
# Read the Popover stories file to see the exact code
cat -n ./src/components/ui/Popover.stories.tsxRepository: getdokan/plugin-ui
Length of output: 2880
Nested <button> elements — PopoverTrigger renders a <button>, and Button is also a <button>.
This produces invalid HTML (<button><button>…</button></button>) and can cause accessibility and click-handling issues. Base UI's Trigger accepts a render prop to customize the rendered element. Use that instead, or pass the Button directly via render.
Possible fix using `render` prop
- <PopoverTrigger>
- <Button variant="outline">Open Popover</Button>
- </PopoverTrigger>
+ <PopoverTrigger render={<Button variant="outline" />}>
+ Open Popover
+ </PopoverTrigger>🤖 Prompt for AI Agents
In `@src/components/ui/Popover.stories.tsx` around lines 21 - 23, The story nests
a Button inside PopoverTrigger which causes invalid nested <button> elements;
update the PopoverTrigger usage (symbol: PopoverTrigger) to render a non-button
wrapper or use its render prop so it doesn't output a <button> around the Button
component (symbol: Button). Replace the current children usage with
PopoverTrigger's render prop (or pass Button directly via render) so the trigger
renders a div/span/fragment or returns the Button element itself, ensuring only
a single interactive element is output and preserving accessibility and click
behavior.
1 participant
Summary by CodeRabbit
New Features
Documentation