refactor: modal v2 added#1377
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
Adds a new token-driven ModalV2 component to the Blend design system, wiring it into the theme/component-token infrastructure and providing Storybook + site demos and tests (closes #1376).
Changes:
- Introduces
ModalV2(desktop dialog + mobile drawer variant), skeleton states, and animation helpers. - Adds
MODALV2component tokens (light/dark) and registers them in theme initialization +useComponentToken. - Adds Storybook stories, site demo integration, and Vitest/jest-axe test coverage.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/blend/lib/context/useComponentToken.ts | Adds MODALV2 token lookup support. |
| packages/blend/lib/context/initComponentTokens.ts | Registers default MODALV2 tokens during theme init. |
| packages/blend/lib/context/ThemeContext.tsx | Adds MODALV2 tokens to theme context typing + defaults. |
| packages/blend/lib/components/ModalV2/utils.ts | New portal utils (currently duplicated/unused). |
| packages/blend/lib/components/ModalV2/modalV2.types.ts | Introduces ModalV2 public types and skeleton types. |
| packages/blend/lib/components/ModalV2/modalV2.tokens.tsx | Adds responsive token types and token selector (light/dark). |
| packages/blend/lib/components/ModalV2/modalV2.light.tokens.ts | Light theme tokens for ModalV2. |
| packages/blend/lib/components/ModalV2/modalV2.dark.tokens.ts | Dark theme tokens for ModalV2. |
| packages/blend/lib/components/ModalV2/modalAnimationV2.tsx | Animation helpers/constants for V2. |
| packages/blend/lib/components/ModalV2/mobileModalV2.tsx | Mobile drawer implementation of ModalV2. |
| packages/blend/lib/components/ModalV2/index.ts | Public exports for ModalV2. |
| packages/blend/lib/components/ModalV2/ModalV2Skeleton.tsx | Skeleton rendering for header/body/footer. |
| packages/blend/lib/components/ModalV2/ModalV2Header.tsx | Header rendering and close button for desktop modal. |
| packages/blend/lib/components/ModalV2/ModalV2Footer.tsx | Footer actions rendering for desktop modal. |
| packages/blend/lib/components/ModalV2/ModalV2.tsx | Desktop modal implementation + mobile switch + portal usage. |
| packages/blend/tests/components/ModalV2/ModalV2.test.tsx | Core behavior tests for ModalV2. |
| packages/blend/tests/components/ModalV2/ModalV2.accessibility.test.tsx | Extensive accessibility tests (axe + keyboard). |
| packages/blend/Design-docs/Modal/ModalDoc.md | New design/usage documentation for ModalV2. |
| apps/storybook/stories/components/Modal/V2/ModalV2.stories.tsx | Storybook stories for ModalV2 variants. |
| apps/site/src/demos/SidebarDemo.tsx | Adds ModalV2 entry to site demo sidebar. |
| apps/site/src/demos/ModalV2Demo.tsx | Adds interactive playground demo for ModalV2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,70 @@ | |||
| // Seleton types | |||
There was a problem hiding this comment.
Typo in the file header comment: "Seleton" should be "Skeleton".
| // Seleton types | |
| // Skeleton types |
| export type ModalV2Props = { | ||
| isOpen: boolean | ||
| isCustom?: boolean | ||
| onClose: () => void | ||
| title?: string | ||
| subtitle?: string | ||
| children: ReactNode | ||
| primaryAction?: ModalV2ButtonAction | ||
| secondaryAction?: ModalV2ButtonAction | ||
| showCloseButton?: boolean | ||
| showHeader?: boolean | ||
| showFooter?: boolean | ||
| closeOnBackdropClick?: boolean | ||
| customHeader?: ReactNode | ||
| customFooter?: ReactNode | ||
| headerSlot?: ReactNode | ||
| showDivider?: boolean | ||
| minWidth?: string | ||
| useDrawerOnMobile?: boolean | ||
| skeleton?: ModalV2BodySkeletonProps | ||
| maxWidth?: string | ||
| maxHeight?: string | ||
| minHeight?: string | ||
| } |
There was a problem hiding this comment.
ModalV2.tsx collects ...props and forwards them to the outer Block (via filterBlockedProps), and the design doc in this PR shows ModalV2Props extending HTMLAttributes<HTMLDivElement>. However, ModalV2Props here does not currently extend any HTML/div attributes type, so consumers can’t pass common props like data-*, className, style, etc. Consider extending React.HTMLAttributes<HTMLDivElement> (or React.ComponentPropsWithoutRef<'div'>) to match the implementation and documentation.
| padding={ | ||
| modalTokens.skeleton.header.paddingTop + | ||
| ' ' + | ||
| modalTokens.skeleton.header.paddingBottom | ||
| } | ||
| flexShrink={0} | ||
| gap={modalTokens.skeleton.header.gap} | ||
| borderBottom={ | ||
| showHeaderDivider | ||
| ? modalTokens.skeleton.header.borderBottom | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
The header skeleton uses padding={paddingTop + ' ' + paddingBottom} which ignores left/right padding tokens (and treats bottom as the horizontal value in 2-value shorthand). This should use all four padding values (or pass the individual padding props) to reflect the token model correctly.
| {showCloseButton && ( | ||
| <DrawerV2Close asChild> | ||
| <XIcon | ||
| size={modalTokens.closeButton.width} | ||
| weight="bold" | ||
| color={ | ||
| modalTokens.closeButton.color | ||
| } | ||
| /> | ||
| </DrawerV2Close> | ||
| )} |
There was a problem hiding this comment.
DrawerV2Close is used with asChild and an XIcon (SVG) as the child. This won’t be keyboard-focusable/clickable like a real button, which can make the mobile modal impossible to close when closeOnBackdropClick={false} (since dismissible is also disabled). Wrap the icon in a <button>/ButtonV2 (or avoid asChild) so the close control is an actual interactive element with an accessible label.
| <DrawerV2 | ||
| open={isOpen} | ||
| onOpenChange={onClose} | ||
| direction="bottom" | ||
| modal={true} | ||
| dismissible={closeOnBackdropClick} | ||
| > |
There was a problem hiding this comment.
onOpenChange={onClose} will invoke onClose for any open state change (including when the drawer opens). Since onOpenChange provides the next open boolean, it’s safer to only call onClose() when open becomes false (close) to avoid spurious close calls on open transitions.
| 'fixed z-[1001] bg-white shadow-xl outline-none flex flex-col overflow-hidden rounded-2xl border border-gray-200 max-h-[85vh]' | ||
| const contentStyle = { | ||
| left: modalTokens.overlay.offset, | ||
| right: modalTokens.overlay.offset, | ||
| bottom: modalTokens.overlay.offset, |
There was a problem hiding this comment.
contentClassName hard-codes visual styles like bg-white, border-gray-200, and shadow-xl, which bypasses the token system (and will look wrong in dark theme). Consider deriving these styles from modalTokens (background, border, shadow, radius, etc.) or using Block/styled-components so the mobile drawer variant stays theme-consistent.
| 'fixed z-[1001] bg-white shadow-xl outline-none flex flex-col overflow-hidden rounded-2xl border border-gray-200 max-h-[85vh]' | |
| const contentStyle = { | |
| left: modalTokens.overlay.offset, | |
| right: modalTokens.overlay.offset, | |
| bottom: modalTokens.overlay.offset, | |
| 'fixed z-[1001] outline-none flex flex-col overflow-hidden max-h-[85vh]' | |
| const contentStyle = { | |
| left: modalTokens.overlay.offset, | |
| right: modalTokens.overlay.offset, | |
| bottom: modalTokens.overlay.offset, | |
| backgroundColor: | |
| (modalTokens as any).content?.backgroundColor ?? | |
| (modalTokens as any).backgroundColor, | |
| boxShadow: | |
| (modalTokens as any).content?.boxShadow ?? | |
| (modalTokens as any).shadow, | |
| borderRadius: | |
| (modalTokens as any).content?.borderRadius ?? | |
| (modalTokens as any).radius, | |
| borderColor: | |
| (modalTokens as any).content?.borderColor ?? | |
| (modalTokens as any).borderColor, | |
| borderWidth: | |
| (modalTokens as any).content?.borderWidth ?? | |
| (modalTokens as any).borderWidth, | |
| borderStyle: | |
| (modalTokens as any).content?.borderStyle ?? | |
| ((modalTokens as any).content?.borderColor ?? | |
| (modalTokens as any).borderColor | |
| ? 'solid' | |
| : undefined), |
| type MobileModalV2Props = ModalV2Props & { | ||
| useDrawerOnMobile?: boolean | ||
| } |
There was a problem hiding this comment.
MobileModalV2Props re-declares useDrawerOnMobile?: boolean, but ModalV2Props already contains this prop. This duplication can be removed to keep the mobile wrapper props type simple.
| type MobileModalV2Props = ModalV2Props & { | |
| useDrawerOnMobile?: boolean | |
| } | |
| type MobileModalV2Props = ModalV2Props |
| const PORTAL_ID = 'blend-modal-portal' | ||
|
|
||
| export const getPortalContainer = (): HTMLElement => { | ||
| let portalContainer = document.getElementById(PORTAL_ID) | ||
|
|
||
| if (!portalContainer) { | ||
| portalContainer = document.createElement('div') | ||
| portalContainer.id = PORTAL_ID | ||
| portalContainer.style.position = 'relative' | ||
| portalContainer.style.zIndex = '101' | ||
| portalContainer.style.pointerEvents = 'auto' | ||
| document.body.appendChild(portalContainer) | ||
| } | ||
|
|
||
| return portalContainer | ||
| } | ||
|
|
||
| export const cleanupPortalContainer = (): void => { | ||
| const portalContainer = document.getElementById(PORTAL_ID) | ||
|
|
||
| if (portalContainer && portalContainer.children.length === 0) { | ||
| document.body.removeChild(portalContainer) | ||
| } | ||
| } |
There was a problem hiding this comment.
utils.ts appears to be an unused duplicate of components/Modal/modal.utils.ts (same PORTAL_ID, getPortalContainer, and cleanupPortalContainer). Nothing in the repo imports components/ModalV2/utils, and ModalV2 currently uses useModal from ../Modal/useModal which already relies on modal.utils. Consider deleting this file to avoid dead code/duplication, or updating ModalV2 to use it (and potentially give ModalV2 its own portal id if that’s the intent).
| const PORTAL_ID = 'blend-modal-portal' | |
| export const getPortalContainer = (): HTMLElement => { | |
| let portalContainer = document.getElementById(PORTAL_ID) | |
| if (!portalContainer) { | |
| portalContainer = document.createElement('div') | |
| portalContainer.id = PORTAL_ID | |
| portalContainer.style.position = 'relative' | |
| portalContainer.style.zIndex = '101' | |
| portalContainer.style.pointerEvents = 'auto' | |
| document.body.appendChild(portalContainer) | |
| } | |
| return portalContainer | |
| } | |
| export const cleanupPortalContainer = (): void => { | |
| const portalContainer = document.getElementById(PORTAL_ID) | |
| if (portalContainer && portalContainer.children.length === 0) { | |
| document.body.removeChild(portalContainer) | |
| } | |
| } | |
| export { getPortalContainer, cleanupPortalContainer } from '../Modal/modal.utils' |
| export type BodySkeletonProps = { | ||
| show?: boolean | ||
| width?: string | ||
| height?: string |
There was a problem hiding this comment.
BodySkeletonProps.height is typed as string, but the component usage passes/derives numeric heights (e.g. defaults like 300 in ModalV2.tsx and 200 in mobileModalV2.tsx). This will either fail type-checking or force unnecessary casting. Consider changing height?: string | number (and aligning with the internal skeleton config type).
| height?: string | |
| height?: string | number |
| padding={ | ||
| modalTokens.header.paddingTop + | ||
| ' ' + | ||
| modalTokens.header.paddingBottom | ||
| } |
There was a problem hiding this comment.
padding is built using only paddingTop and paddingBottom (two-value shorthand uses vertical/horizontal). This ignores paddingLeft/paddingRight tokens and will produce incorrect horizontal padding if the token values ever diverge. Prefer passing paddingTop/paddingRight/paddingBottom/paddingLeft explicitly (as done elsewhere) or construct a 4-value padding string using all four token values.
| padding={ | |
| modalTokens.header.paddingTop + | |
| ' ' + | |
| modalTokens.header.paddingBottom | |
| } | |
| paddingTop={modalTokens.header.paddingTop} | |
| paddingRight={modalTokens.header.paddingRight} | |
| paddingBottom={modalTokens.header.paddingBottom} | |
| paddingLeft={modalTokens.header.paddingLeft} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }: { | ||
| title?: string | ||
| subtitle?: string | ||
| onClose: () => void | ||
| showCloseButton?: boolean | ||
| headerSlot?: React.ReactNode | ||
| showDivider?: boolean |
| import type { ModalV2SkeletonProps } from './modalV2.types' | ||
|
|
||
| const ModalV2Skeleton: React.FC<ModalV2SkeletonProps> = ({ |
| @@ -0,0 +1,70 @@ | |||
| // Seleton types | |||
| export type BodySkeletonProps = { | ||
| show?: boolean | ||
| width?: string | ||
| height?: string |
| export type ModalV2Props = { | ||
| isOpen: boolean | ||
| isCustom?: boolean | ||
| onClose: () => void | ||
| title?: string | ||
| subtitle?: string | ||
| children: ReactNode | ||
| primaryAction?: ModalV2ButtonAction | ||
| secondaryAction?: ModalV2ButtonAction | ||
| showCloseButton?: boolean | ||
| showHeader?: boolean | ||
| showFooter?: boolean | ||
| closeOnBackdropClick?: boolean | ||
| customHeader?: ReactNode | ||
| customFooter?: ReactNode | ||
| headerSlot?: ReactNode | ||
| showDivider?: boolean | ||
| minWidth?: string | ||
| useDrawerOnMobile?: boolean | ||
| skeleton?: ModalV2BodySkeletonProps | ||
| maxWidth?: string | ||
| maxHeight?: string | ||
| minHeight?: string | ||
| } |
| padding={ | ||
| modalTokens.skeleton.header.paddingTop + | ||
| ' ' + | ||
| modalTokens.skeleton.header.paddingBottom |
| const PORTAL_ID = 'blend-modal-portal' | ||
|
|
||
| export const getPortalContainer = (): HTMLElement => { | ||
| let portalContainer = document.getElementById(PORTAL_ID) | ||
|
|
||
| if (!portalContainer) { | ||
| portalContainer = document.createElement('div') | ||
| portalContainer.id = PORTAL_ID | ||
| portalContainer.style.position = 'relative' | ||
| portalContainer.style.zIndex = '101' | ||
| portalContainer.style.pointerEvents = 'auto' | ||
| document.body.appendChild(portalContainer) | ||
| } | ||
|
|
||
| return portalContainer | ||
| } | ||
|
|
||
| export const cleanupPortalContainer = (): void => { | ||
| const portalContainer = document.getElementById(PORTAL_ID) | ||
|
|
||
| if (portalContainer && portalContainer.children.length === 0) { | ||
| document.body.removeChild(portalContainer) | ||
| } | ||
| } |
| <XIcon | ||
| size={modalTokens.closeButton.width} | ||
| weight="bold" | ||
| color={ | ||
| modalTokens.closeButton.color | ||
| } | ||
| /> |
| const contentClassName = | ||
| 'fixed z-[1001] bg-white shadow-xl outline-none flex flex-col overflow-hidden rounded-2xl border border-gray-200 max-h-[85vh]' | ||
| const contentStyle = { | ||
| left: modalTokens.overlay.offset, | ||
| right: modalTokens.overlay.offset, | ||
| bottom: modalTokens.overlay.offset, |
| - **Responsive**: Desktop modal and mobile drawer variants | ||
| - **Animations**: Smooth enter/exit transitions (300ms) | ||
| - **Skeleton Loading**: Built-in skeleton state for header, body, and footer | ||
| - **Accessibility**: Full ARIA support, keyboard navigation, focus management |
Summary
Screen.Recording.2026-04-28.at.7.39.34.PM.mov
modal v2 added
Issue Ticket
Closes #1376