[WEB-4684] chore: dialog component enhancements#7606
[WEB-4684] chore: dialog component enhancements#7606sriramveeraghanta merged 8 commits intopreviewfrom
Conversation
WalkthroughRemoves dialog enums from constants, reimplements Dialog as a typed, memoized compound component (Dialog with Panel and Title) exposing width and position tokens and ARIA attributes, adds a named z-index scale to Tailwind config, and makes a formatting-only change in Popover positioning code. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User (renders)
participant D as Dialog (Root)
participant P as Dialog.Panel
participant Port as Portal
participant O as Overlay
participant Pop as Popup (role="dialog")
U->>D: Instantiate Dialog
D->>P: Compose Panel (width, position props)
P->>Port: Mount into Portal
Port->>O: Render Overlay (OVERLAY_CLASSNAME)
O->>Pop: Render Popup (BASE_CLASSNAME, role="dialog", aria-modal="true")
Note over P,Pop: Class composition merges base + position + width tokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ 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 |
…g-component-enhancements
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/propel/src/dialog/root.tsx (4)
22-37: Type simplicity: avoid redeclaring children where possibleYou can simplify these props with
PropsWithChildreninstead of explicitly redeclaringchildrenfor each interface.Example:
-export interface DialogProps extends React.ComponentProps<typeof BaseDialog.Root> { - children: React.ReactNode; -} +export type DialogProps = React.PropsWithChildren< + React.ComponentProps<typeof BaseDialog.Root> +>; -export interface DialogPanelProps extends React.ComponentProps<typeof BaseDialog.Popup> { - width?: EDialogWidth; - position?: DialogPosition; - children: React.ReactNode; -} +export type DialogPanelProps = React.PropsWithChildren< + React.ComponentProps<typeof BaseDialog.Popup> +> & { + width?: EDialogWidth; + position?: DialogPosition; +}; -export interface DialogTitleProps extends React.ComponentProps<typeof BaseDialog.Title> { - children: React.ReactNode; -} +export type DialogTitleProps = React.PropsWithChildren< + React.ComponentProps<typeof BaseDialog.Title> +>;
40-42: Minor: avoid unnecessarycninvocation for static class list
OVERLAY_CLASSNAMEis static; callingcnisn’t needed and adds runtime work (tiny, but avoidable).Apply this diff:
-const OVERLAY_CLASSNAME = cn("fixed inset-0 z-backdrop bg-custom-backdrop"); +const OVERLAY_CLASSNAME = "fixed inset-0 z-backdrop bg-custom-backdrop";
77-98: Optional: theuseMemohere is unnecessary
getPositionClassNamesis a cheap, pure function; memoization doesn’t buy much and slightly complicates the code.Apply this diff to compute directly:
- const positionClassNames = React.useMemo(() => getPositionClassNames(position), [position]); + const positionClassNames = getPositionClassNames(position);Optionally, consider exposing
aria-labelledbyonDialogPanelPropsand wiring it toDialogTitleids for stronger A11Y guarantees. Not blocking.
70-76: Remove unused internalDialogTriggercomponentNo exports or references to
DialogTriggerwere found anywhere in the repo. You can safely delete it to reduce bundle size and surface area:
- File: packages/propel/src/dialog/root.tsx
- Remove lines 70–76 (the
DialogTriggerdefinition and itsdisplayName)
📜 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/dialog/constants.ts(0 hunks)packages/propel/src/dialog/root.tsx(1 hunks)packages/tailwind-config/tailwind.config.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/propel/src/dialog/constants.ts
🧰 Additional context used
🪛 Biome (2.1.2)
packages/propel/src/dialog/root.tsx
[error] 44-44: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 GitHub Check: Build and lint web apps
packages/propel/src/dialog/root.tsx
[failure] 44-44:
Unexpected block statement surrounding arrow body; move the returned value immediately after the =>
🪛 GitHub Actions: Build and lint web apps
packages/propel/src/dialog/root.tsx
[error] 44-44: ESLint (eslint . --max-warnings 7): Unexpected block statement surrounding arrow body; move the returned value immediately after the =>. (arrow-body-style)
🔇 Additional comments (3)
packages/tailwind-config/tailwind.config.js (1)
445-458: Z-index design tokens look solid and align with dialog layering needsThe scale is clear, non-overlapping, and maps well to the new
z-backdropandz-modalusages in the dialog. This should help avoid ad-hoc values going forward.packages/propel/src/dialog/root.tsx (2)
9-20: Width tokens are well-chosenThe
EDialogWidthvalues map cleanly to Tailwind’smax-w-*utilities and provide a sensible defaulting story (mobilew-full, wider screens constrained). No issues here.
58-62: Overlay composition and z-index choice look good
z-backdroppairs correctly withz-modal, and the Backdrop and Popup ordering within the Portal is sound.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/propel/src/dialog/root.tsx (1)
44-51: Fix: hook used at module scope (breaks lint) — convert to a plain function
React.useCallbackcannot be used at module scope. This violates the hooks rule and is failing lint. It’s also unnecessary here—use a plain function with an expression body.Apply this diff:
-const getPositionClassNames = React.useCallback( - (position: DialogPosition) => - cn("isolate fixed z-modal", { - "top-1/2 left-1/2 -translate-x-1/2 -translate-y-1/2": position === "center", - "top-8 left-1/2 -translate-x-1/2": position === "top", - }), - [] -); +const getPositionClassNames = (position: DialogPosition) => + cn("isolate fixed z-modal", { + "top-1/2 left-1/2 -translate-x-1/2 -translate-y-1/2": position === "center", + "top-8 left-1/2 -translate-x-1/2": position === "top", + });
🧹 Nitpick comments (1)
packages/propel/src/dialog/root.tsx (1)
81-81: Simplify: drop useMemo; compute class once inline
getPositionClassNames(position)is pure and cheap; memoization adds complexity with no measurable gain.Apply this diff:
- const positionClassNames = React.useMemo(() => getPositionClassNames(position), [position]); return ( <DialogPortal> <DialogOverlay /> <BaseDialog.Popup ref={ref} data-slot="dialog-content" - className={cn(BASE_CLASSNAME, positionClassNames, width, className)} + className={cn(BASE_CLASSNAME, getPositionClassNames(position), width, className)} role="dialog" aria-modal="true" {...props} >Also applies to: 88-88
📜 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 (1)
packages/propel/src/dialog/root.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
packages/propel/src/dialog/root.tsx
[error] 44-44: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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 (6)
packages/propel/src/dialog/root.tsx (6)
9-19: Width tokens mapping looks solidGood coverage from sm:max-w-sm up to sm:max-w-7xl; sensible defaults for responsive dialog widths.
53-70: Portal/Overlay composition LGTMClear separation of responsibilities with memoized wrappers and data-slot tagging for testing/styling.
101-107: DialogTitle wrapper LGTMConcise wrapper over BaseDialog.Title with sensible typography classes.
109-116: Typed compound component pattern is correctNice use of Object.assign with precise static property typing for Dialog.Panel and Dialog.Title.
40-41: zIndex tokens verified (backdrop < modal)
The Tailwind config (packages/tailwind-config/tailwind.config.js) defines:
- backdrop: 90
- modal: 100
Their ordering satisfies backdrop < modal. No further changes needed.
89-90: Keep ARIA attributes on Dialog.PopupBase UI’s
Dialog.Popupdoes not addrole="dialog"oraria-modalby default—those attributes must be provided for proper accessibility. Including them here is correct and not redundant.
…g-component-enhancements
3 participants
Description
This PR includes following changes:
Type of Change
Summary by CodeRabbit
New Features
Breaking Changes
Accessibility
Style