[WEB-4980] dev: propel modal portal component#7851
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 portal feature under packages/propel: PortalWrapper and ModalPortal components with types, constants, Storybook stories, a src aggregator, a new public export "./portal" in package.json, and includes src/portal/index.ts in tsdown build entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant ModalPortal
participant PortalWrapper
participant DOM
App->>ModalPortal: render(isOpen, width, position, ...)
alt isOpen = false
ModalPortal-->>App: returns null
else isOpen = true
ModalPortal->>ModalPortal: compute classes, setup refs & listeners
ModalPortal->>PortalWrapper: mount overlay + content into portalId
PortalWrapper->>DOM: find or create container (DEFAULT_PORTAL_ID)
PortalWrapper-->>ModalPortal: container ready -> portal mounted
par User interactions
App->>ModalPortal: press Escape
ModalPortal->>App: call onClose() if closeOnEscape
App->>ModalPortal: click overlay
ModalPortal->>App: call onClose() if closeOnOverlayClick
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 20. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new portal component system for the Propel UI library, enabling modal and overlay functionality that renders outside the normal DOM tree. The portal system provides reusable components for creating modals, overlays, and other UI elements that need to appear above other content.
- Adds
ModalPortalcomponent with configurable positioning, sizing, and overlay behavior - Implements
PortalWrapperas a low-level portal utility for DOM manipulation - Includes comprehensive Storybook documentation with interactive examples
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/propel/tsdown.config.ts |
Adds portal module to build configuration |
packages/propel/package.json |
Exports portal components in package.json exports |
packages/propel/src/portal/types.ts |
Defines TypeScript interfaces for portal components |
packages/propel/src/portal/constants.ts |
Defines enums and styling constants for portal positioning and sizing |
packages/propel/src/portal/portal-wrapper.tsx |
Implements low-level portal wrapper component |
packages/propel/src/portal/modal-portal.tsx |
Implements modal portal component with accessibility features |
packages/propel/src/portal/portal.stories.tsx |
Provides Storybook documentation and interactive examples |
packages/propel/src/portal/index.ts |
Exports all portal-related components and types |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
packages/propel/src/portal/constants.ts (1)
27-29: Overlay z-index constant is unused.Consider using OVERLAY_Z_INDEX in ModalPortal to make stacking explicit and consistent with MODAL_Z_INDEX.
packages/propel/src/portal/portal-wrapper.tsx (3)
1-12: Unify props typing and avoid SSR warnings from useLayoutEffect.
- Don’t redeclare PortalWrapperProps locally; import it from ./types to keep a single source of truth.
- Using useLayoutEffect on the server emits warnings. Prefer an isomorphic layout effect.
Apply:
-import React, { useLayoutEffect, useState, useMemo } from "react"; +import React, { useEffect, useLayoutEffect, useState, useMemo } from "react"; import { createPortal } from "react-dom"; import { DEFAULT_PORTAL_ID } from "./constants"; +import type { PortalWrapperProps } from "./types"; -type PortalWrapperProps = { - children: React.ReactNode; - portalId?: string; - fallbackToDocument?: boolean; - className?: string; - onMount?: () => void; - onUnmount?: () => void; -}; +const useIsomorphicLayoutEffect = typeof window !== "undefined" ? useLayoutEffect : useEffect;
36-61: Swap to isomorphic layout effect.This removes the “useLayoutEffect does nothing on the server” warning.
- useLayoutEffect(() => { + useIsomorphicLayoutEffect(() => { // Ensure we're in browser environment if (typeof window === "undefined") return; @@ - }, [portalId, onMount, onUnmount]); + }, [portalId, onMount, onUnmount]);
79-81: Avoid unsafe cast to ReactElement.content can be non-element nodes. Prefer returning content directly (ReactNode) or ensure wrapping.
- if (fallbackToDocument) { - return content as React.ReactElement; - } + if (fallbackToDocument) { + return <>{content}</>; + }packages/propel/src/portal/portal.stories.tsx (2)
192-207: Mixing stories for different components under one default meta.BasicPortal renders PortalWrapper but the file’s default meta is for ModalPortal. Split into portal-wrapper.stories.tsx or set a separate default export for PortalWrapper in a new file to keep controls/docs accurate.
171-172: String transform nit.EPortalWidth values are hyphenated (e.g., "three-quarter"); replace("_", " ") is no-op. Consider normalizing labels with a dedicated util.
Also applies to: 180-181
packages/propel/src/portal/modal-portal.tsx (2)
13-27: Duplicate props type with ./types.This local ModalPortalProps duplicates the public interface in types.ts. Import and reuse to avoid drift.
-type ModalPortalProps = { +import type { ModalPortalProps } from "./types"; +type ModalPortalProps = { children: React.ReactNode; isOpen: boolean; onClose?: () => void; portalId?: string; className?: string; overlayClassName?: string; contentClassName?: string; width?: EPortalWidth; position?: EPortalPosition; fullScreen?: boolean; showOverlay?: boolean; closeOnOverlayClick?: boolean; closeOnEscape?: boolean; };If you prefer not to re-export, at least keep it in sync with the public type.
60-60: contentRef is unused without focus features.If you don’t implement focus management, remove the ref to avoid dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/propel/package.json(1 hunks)packages/propel/src/portal/constants.ts(1 hunks)packages/propel/src/portal/index.ts(1 hunks)packages/propel/src/portal/modal-portal.tsx(1 hunks)packages/propel/src/portal/portal-wrapper.tsx(1 hunks)packages/propel/src/portal/portal.stories.tsx(1 hunks)packages/propel/src/portal/types.ts(1 hunks)packages/propel/tsdown.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/propel/src/portal/portal-wrapper.tsx (2)
packages/propel/src/portal/types.ts (1)
PortalWrapperProps(8-13)packages/propel/src/portal/constants.ts (1)
DEFAULT_PORTAL_ID(27-27)
packages/propel/src/portal/portal.stories.tsx (1)
packages/propel/src/portal/portal-wrapper.tsx (1)
PortalWrapper(25-84)
⏰ 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: Build and lint web apps
🔇 Additional comments (4)
packages/propel/tsdown.config.ts (1)
24-24: Build entry for portal added — looks good.Including "src/portal/index.ts" ensures the new module is built and published.
packages/propel/package.json (1)
38-38: New export "./portal" — OK.Matches the added tsdown entry and index re-exports. Should resolve to dist/portal/index.(js|d.ts).
packages/propel/src/portal/constants.ts (1)
14-25: Class tokens: verify Tailwind presets for min/max width utilities.min-w-80 / min-w-96 etc. require custom Tailwind config. Confirm these exist in @plane/tailwind-config to avoid missing styles.
packages/propel/src/portal/index.ts (1)
1-4: Public aggregator is clean and cohesive.Re-exports provide a tidy surface for consumers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/propel/src/portal/portal-wrapper.tsx (1)
1-1: PreferuseEffecthere to stay SSR-safe.Using
useLayoutEffectin a component that can render during SSR will emit warnings even though the body short-circuits in browsers. Swapping touseEffectkeeps the behavior while avoiding the server-side complaint.-import React, { useLayoutEffect, useState, useMemo } from "react"; +import React, { useEffect, useState, useMemo } from "react"; @@ - useLayoutEffect(() => { + useEffect(() => { // Ensure we're in browser environment if (typeof window === "undefined") return; @@ - }, [portalId, onMount, onUnmount]); + }, [portalId, onMount, onUnmount]);Also applies to: 28-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/propel/src/portal/constants.ts(1 hunks)packages/propel/src/portal/modal-portal.tsx(1 hunks)packages/propel/src/portal/portal-wrapper.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/propel/src/portal/constants.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/propel/src/portal/modal-portal.tsx (3)
packages/propel/src/portal/types.ts (1)
ModalPortalProps(15-27)packages/propel/src/portal/constants.ts (4)
DEFAULT_PORTAL_ID(27-27)PORTAL_WIDTH_CLASSES(14-19)PORTAL_POSITION_CLASSES(21-25)MODAL_Z_INDEX(28-28)packages/propel/src/portal/portal-wrapper.tsx (1)
PortalWrapper(17-76)
packages/propel/src/portal/portal-wrapper.tsx (2)
packages/propel/src/portal/types.ts (1)
PortalWrapperProps(8-13)packages/propel/src/portal/constants.ts (1)
DEFAULT_PORTAL_ID(27-27)
⏰ 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 (1)
packages/propel/src/portal/modal-portal.tsx (1)
66-72: Escape handler should only exist while the modal is open.
handleEscapestays attached even after the modal is closed, so pressing Escape anywhere keeps firingonClose. Please gate the listener behindisOpen.- useEffect(() => { - document.addEventListener("keydown", handleEscape); - return () => { - document.removeEventListener("keydown", handleEscape); - }; - }, [handleEscape]); + useEffect(() => { + if (!isOpen) { + return; + } + document.addEventListener("keydown", handleEscape); + return () => { + document.removeEventListener("keydown", handleEscape); + }; + }, [isOpen, handleEscape]);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/propel/src/portal/portal.stories.tsx (4)
82-83: Avoid overstating a11y; remove “Tab/focus management” claims or implement trap.ModalPortal doesn’t implement focus trap or managed focus/return-to-trigger. Adjust story copy to avoid misleading guidance.
Apply:
- description = "This is a modal portal component with full accessibility support. Try pressing Tab to navigate through elements or Escape to close.", + description = "Accessible modal portal demo. Supports closing via Escape key and clicking the overlay.",- description="A standard modal with all default settings. Demonstrates focus management, keyboard navigation, and accessibility features." + description="A standard modal with default settings. Supports closing via Escape key and overlay click."Also applies to: 121-124
62-64: Use stable enum type for buttonVariant instead of inferring from Button.Using Parameters is brittle (forwardRefs/HOCs can break it). Prefer the enum you already export.
}: Omit<Parameters<typeof ModalPortal>[0], "isOpen" | "onClose"> & { buttonText?: string; - buttonVariant?: Parameters<typeof Button>[0]["variant"]; + buttonVariant?: EButtonVariant; }) => {
69-71: Add ARIA to the trigger button.Expose dialog intent and expanded state.
- <Button variant={buttonVariant} onClick={() => setIsOpen(true)}> + <Button + variant={buttonVariant} + onClick={() => setIsOpen(true)} + aria-haspopup="dialog" + aria-expanded={isOpen} + >
191-206: PortalWrapper story won’t appear under its own “Components/Portal/PortalWrapper” title.Storybook uses a single default meta per file; per‑story title override isn’t supported. Move PortalWrapper into its own CSF file with its own default export to get the desired sidebar grouping.
Follow-up: create packages/propel/src/portal/portal-wrapper.stories.tsx with default meta for PortalWrapper and move BasicPortal there.
packages/propel/src/portal/types.ts (1)
16-28: Add ARIA props to support accessible labeling.Enable consumers to set aria-modal, aria-labelledby, and aria-describedby on the dialog wrapper.
export interface ModalPortalProps extends BasePortalProps { isOpen: boolean; onClose?: () => void; portalId?: string; overlayClassName?: string; contentClassName?: string; width?: EPortalWidth; position?: EPortalPosition; fullScreen?: boolean; showOverlay?: boolean; closeOnOverlayClick?: boolean; closeOnEscape?: boolean; + ariaModal?: boolean; + ariaLabelledby?: string; + ariaDescribedby?: string; }Follow-up: pass these through to the element with role="dialog" in modal-portal.tsx.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/propel/src/portal/modal-portal.tsx(1 hunks)packages/propel/src/portal/portal.stories.tsx(1 hunks)packages/propel/src/portal/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/propel/src/portal/modal-portal.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/portal/portal.stories.tsx (2)
packages/propel/src/portal/modal-portal.tsx (1)
ModalPortal(29-110)packages/propel/src/portal/portal-wrapper.tsx (1)
PortalWrapper(17-76)
⏰ 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: Build and lint web apps
🔇 Additional comments (1)
packages/propel/src/portal/types.ts (1)
1-1: Good fix: type-only React imports.Resolves previous React namespace/type issues.
Please run type-check to confirm no downstream breakages.
Description
This PR includes propel modal portal component.
Media
Summary by CodeRabbit
New Features
Documentation
Chores