CRAFT-1734: Add Dialog component #428
Conversation
🦋 Changeset detectedLatest commit: 1ca3fac The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Replaced Dialog components with Modal components in the AppNavBarCreateButton and AppNavBarSearch components for consistency and improved functionality. - Updated Dialog imports to LegacyDialog in the navigation components. - Added new Modal component and its related types, slots, and stories to enhance the modal functionality. - Updated documentation to reflect the changes in the Dialog and Modal components. All tests pass, ensuring no breaking changes introduced.
…nents - Updated AppNavBarCreateButton and AppNavBarSearch components to utilize the new Modal component, enhancing consistency and functionality. - Adjusted imports and component structure to reflect the transition from LegacyDialog to Modal. - Ensured all related functionality remains intact with no breaking changes introduced. All tests pass successfully.
…lity - Consolidated imports and improved formatting across various modal components for better readability and consistency. - Updated Modal and Drawer components to utilize new conditions for state management, enhancing animation handling. - Refactored stories and documentation to reflect the latest changes in component structure and accessibility features. - Ensured all components maintain proper accessibility standards and functionality. All tests pass successfully, confirming no breaking changes introduced.
- Added IntlProvider to the App component to enable internationalization features. - Updated AppNavBarCreateButton and AppNavBarSearch components to utilize the new Modal structure with improved accessibility. - Refactored ModalTrigger to support asChild prop for better component flexibility. All tests pass successfully, ensuring no breaking changes introduced.
- Removed the Modal component and its related files, transitioning to a Dialog-based architecture for improved accessibility and consistency. - Updated Dialog components to utilize new slot-based structure, enhancing flexibility and maintainability. - Refactored stories and documentation to reflect the changes in component structure and accessibility features. - Ensured all components maintain proper accessibility standards and functionality. All tests pass successfully, confirming no breaking changes introduced.
…ove structure - Refactored Dialog components to replace forwardRef with local refs, enhancing clarity and maintainability. - Consolidated imports and improved formatting across various dialog components for better readability. - Updated stories to reflect changes in component structure and ensure proper accessibility standards. All tests pass successfully, confirming no breaking changes introduced.
… variants - Updated Dialog components to eliminate the size prop, enhancing simplicity and usability. - Changed DialogCloseTrigger from a button to a div for better positioning control. - Refactored stories to align with the new component structure and removed unnecessary size-related examples. - Ensured all components maintain accessibility standards and functionality. All tests pass successfully, confirming no breaking changes introduced.
- Introduced a new story, ButtonAsTrigger, demonstrating how to trigger the Dialog component using a custom Button instead of the default trigger. - Enhanced the dialog's functionality by maintaining the Button's styling while providing dialog trigger capabilities. - Included interaction tests for opening and closing the dialog, as well as footer button interactions to ensure proper functionality. All tests pass successfully, confirming no breaking changes introduced.
… management - Introduced DialogContext to manage configuration for Dialog components, enhancing the ability to pass props down to child components. - Updated Dialog.Root to utilize the new context, simplifying prop management and improving component structure. - Refactored Dialog.Content to extract configuration from context instead of props, streamlining the component's API. - Adjusted stories to reflect the new context-based structure and ensure proper functionality. All tests pass successfully, confirming no breaking changes introduced.
…ration - Eliminated the motionPreset prop and related configurations from Dialog components, simplifying the API and enhancing usability. - Updated dialog stories to reflect the removal of motion presets, ensuring clarity in usage examples. - Refactored dialog components to improve structure and maintainability, while ensuring all components adhere to accessibility standards. All tests pass successfully, confirming no breaking changes introduced.
…context support - Introduced a new Heading component that integrates Chakra UI's Heading while utilizing the HeadingContext for improved prop management. - Defined HeadingProps interface to extend ChakraHeadingProps, adding support for ref and slot attributes. - Updated the component to handle context properties and ensure proper rendering with accessibility in mind. All tests pass successfully, confirming no breaking changes introduced.
…ted documentation - Eliminated the Dialog.Description component to streamline the dialog API, enhancing usability and reducing complexity. - Updated documentation to reflect the removal of the description prop and adjusted examples accordingly. - Refactored Dialog stories to remove references to the description, ensuring clarity in usage examples. - Improved the Dialog component structure by consolidating props and enhancing accessibility features. All tests pass successfully, confirming no breaking changes introduced.
- Integrated useDialogRootContext to manage scroll behavior within Dialog.Body. - Added tabIndex property to enable keyboard navigation when scrollBehavior is set to "inside". - Updated DialogBodySlot to utilize new defaultProps for improved accessibility and user experience. All tests pass successfully, confirming no breaking changes introduced.
…og structure - Removed the Dialog.Backdrop component and replaced it with a modal overlay for improved functionality and clarity. - Updated dialog slot recipes to reflect the new modal and modalOverlay slots, enhancing the dialog's configuration. - Refactored Dialog.Content to utilize the new modal overlay, streamlining the component's API. - Adjusted stories to demonstrate the updated dialog structure and ensure proper functionality. All tests pass successfully, confirming no breaking changes introduced.
misama-ct
requested review from
stephsprinkle,
jaikamat,
ddouglasz,
valoriecarli,
tylermorrisford and
ByronDWall
and removed request for
a team
| // merge the local ref with a potentially forwarded ref | ||
| const ref = useObjectRef(mergeRefs(localRef, forwardedRef)); | ||
|
|
||
| const { scrollBehavior } = useDialogRootContext(); |
There was a problem hiding this comment.
I might be missing why this is configurable, but a part of me feels like we should be opinionated about scroll behavior. Do we need to expose this to consumers?
There was a problem hiding this comment.
There is a story illustrating the difference. Sometimes you want the action-buttons to always be visible, sometimes, you want to force people to read everything and only then take an action.
Not dying on that hill though, @commercetools/craft-team-fe if someone else feels this is unnecessary I can also get rid of it.
| // create a local ref (because the consumer may not provide a forwardedRef) | ||
| const localRef = useRef<HTMLDivElement>(null); | ||
| // merge the local ref with a potentially forwarded ref | ||
| const ref = useObjectRef(mergeRefs(localRef, forwardedRef)); |
There was a problem hiding this comment.
I feel stupid asking this, but I have to because it's a pattern I see everywhere and it almost feels automatic to me to do it.
Why do we do this?! Or rather, why do we use RAC's and Chakra's utils, what problem does this solve?
There was a problem hiding this comment.
If I remember correctly, there was some difference in how react-aria handles refs and chakra does that.
So you couldn't really put the forwardedRef directly on a react-aria component. And since the user may or may not have provided this forwardedRef, you create a local ref that merges a potential provided one and attach this one to the RA component instead. This localRef also allows you to work with the element internally (e.g. measure dimensions), while still providing access to that element to consumers via the forwardedRef.
@ByronDWall please sanity check what I just said.
There was a problem hiding this comment.
Hm, not getting any squiggly lines anymore when wiring the forwardedRef to a ra-component directly. I'll chekc where I can simplify things.
| variant="solid" | ||
| tone="primary" | ||
| isDisabled={!username || !password || !loginReason} | ||
| onClick={handleSubmit} |
There was a problem hiding this comment.
nit: needs to be onPress or it double-fires
| /** | ||
| * Dialog with nested dialogs to test z-index management and proper stacking behavior. | ||
| */ | ||
| export const NestedDialogs: Story = { |
There was a problem hiding this comment.
I assume nesting works fine with click-outside dismissal for each nested dialog? Just covering bases here.
There was a problem hiding this comment.
Yes. I just added the isDismissable property and tried it out. IS that acase that's worth testing for?
| * Whether to render as a child element (use children directly as the trigger) | ||
| * @default false | ||
| */ | ||
| asChild?: boolean; |
There was a problem hiding this comment.
I know we're setting this precedent, but I'm still not sold on this for consumers to understand. We don't have a good internal "detect a RAC button" solution, so it feels like we're pawning this off to the consumer, to understand our abstraction.
It is what it is 🤷♂️
| top: 0, | ||
| w: "100vw", | ||
| h: "100dvh", | ||
| backdropFilter: "blur({sizes.100})", |
There was a problem hiding this comment.
Looks phenomenal! But does this have performance implications for clients that we need to worry about? I seem to remember these CSS filters having less-than-good performance implications for apps
There was a problem hiding this comment.
The blur calculation happens once and the animation is scoped to the opacity property, so in this case performance issues are very unlikely. However, there certainly are ways to wreck the performance.
If you would add a transition property that uses all instead of a scoped property (width, height, opacity etc.), and the blur-value would change between start and end, it would try animating the backdrop-blur property as well.
Instead of 1 calculation it would be many calculations. Assuming a 60Hz screen, it would be 60 calculations per second (or less if your computer can't handle it).
packages/nimbus/src/components/dialog/components/dialog.body.tsx
Outdated
Show resolved
Hide resolved
| const { ref: forwardedRef, children, ...restProps } = props; | ||
|
|
||
| // create a local ref (because the consumer may not provide a forwardedRef) | ||
| const localRef = useRef<HTMLDivElement>(null); |
There was a problem hiding this comment.
If the consumer doesn't provide a ref, why do we need the local ref? we aren't using it here
There was a problem hiding this comment.
Technically its not needed, you're right. It's just that during development, when starting, I don't know what is needed and I don't want to spend time reasoning about it with every subcomponent, so I just cover all cases with this 2 lines.
If you're bothered I can get rid of the localRef's, but I think it's also great to have the same implementation everywhere and not having to think about it every time.
I simplified dialog.body and all other sub-components. Apparently it's not even necessary anymore to transform a chakra forwardedRef into a react-aria compatible ref as the forwardedRef just works.
| export const DialogCloseTrigger = (props: DialogCloseTriggerProps) => { | ||
| const { | ||
| ref: forwardedRef, | ||
| "aria-label": ariaLabel = "Close dialog", |
There was a problem hiding this comment.
the default label should be an intl message
| * | ||
| * The trigger element that opens the dialog when activated. | ||
| */ | ||
| export interface DialogTriggerProps extends ComponentProps<"button"> { |
There was a problem hiding this comment.
Why are most of the compound component prop types extending ComponentProps instead of their slot props? Do we not want them to be stylable via props?
There was a problem hiding this comment.
Good catch, this is definitely not right.
| export const Dialog: DialogComponents = { | ||
| Root: ChakraDialog.Root, | ||
| Trigger: ChakraDialog.Trigger, | ||
| export const Dialog = { |
There was a problem hiding this comment.
My suggestion for jsdoc comments on compound components is to declare the comments in the compound object declaration instead of the individual component files. This ensures that consumers see the jsdoc hints when peeking the type definitions (see comment in dialog.body.tsx.
| export const Dialog = { | |
| export const Dialog = { | |
| /** | |
| * ### Dialog.Root | |
| * | |
| * Root component that provides context and state management for the dialog. | |
| * Uses React Aria's DialogTrigger for accessibility and keyboard interaction. | |
| * | |
| * This component must wrap all dialog parts (Trigger, Content, etc.) and provides | |
| * the dialog open/close state and variant styling context. | |
| * | |
| * @example | |
| * ```tsx | |
| * <Dialog.Root> | |
| * <Dialog.Trigger>Open Dialog</Dialog.Trigger> | |
| * <Dialog.Content> | |
| * <Dialog.Header> | |
| * <Dialog.Title>Dialog Title</Dialog.Title> | |
| * </Dialog.Header> | |
| * <Dialog.Body>Dialog content</Dialog.Body> | |
| * </Dialog.Content> | |
| * </Dialog.Root> | |
| * ``` | |
| */ | |
| Root: DialogRoot, // MUST BE FIRST - primary entry point | |
| /** | |
| * ### Dialog.Trigger | |
| * | |
| * Trigger element that opens the dialog when activated. | |
| * Uses React Aria's Button for accessibility and keyboard support. | |
| * | |
| * @example | |
| * ```tsx | |
| * <Dialog.Root> | |
| * <Dialog.Trigger>Open Dialog</Dialog.Trigger> | |
| * <Dialog.Content>...</Dialog.Content> | |
| * </Dialog.Root> | |
| * ``` | |
| */ | |
| Trigger: DialogTrigger, | |
| /** | |
| * ### Dialog.Content | |
| * | |
| * Main dialog content container that wraps React Aria's Modal and Dialog. | |
| * Handles portalling, backdrop, positioning, and content styling. | |
| * | |
| * This component creates the dialog overlay, positions the content, and provides | |
| * accessibility features like focus management and keyboard dismissal. | |
| * | |
| * @example | |
| * ```tsx | |
| * <Dialog.Root> | |
| * <Dialog.Trigger>Open Dialog</Dialog.Trigger> | |
| * <Dialog.Content size="md" placement="center"> | |
| * <Dialog.Header> | |
| * <Dialog.Title>Title</Dialog.Title> | |
| * </Dialog.Header> | |
| * <Dialog.Body>Content</Dialog.Body> | |
| * <Dialog.Footer>Actions</Dialog.Footer> | |
| * </Dialog.Content> | |
| * </Dialog.Root> | |
| * ``` | |
| */ | |
| Content: DialogContent, | |
| /** | |
| * ### Dialog.Header | |
| * | |
| * Header section of the dialog content. | |
| * Typically contains the title and close button. | |
| * | |
| * @example | |
| * ```tsx | |
| * <Dialog.Content> | |
| * <Dialog.Header> | |
| * <Dialog.Title>Dialog Title</Dialog.Title> | |
| * <Dialog.CloseTrigger /> | |
| * </Dialog.Header> | |
| * <Dialog.Body>...</Dialog.Body> | |
| * </Dialog.Content> | |
| * ``` | |
| */ | |
| Header: DialogHeader, | |
| /** | |
| * ### Dialog.Body | |
| * | |
| * Main body content section of the dialog. | |
| * Contains the primary dialog content and handles overflow/scrolling. | |
| * | |
| * @example | |
| * ```tsx | |
| * <Dialog.Content> | |
| * <Dialog.Header>...</Dialog.Header> | |
| * <Dialog.Body> | |
| * <p>This is the main content of the dialog.</p> | |
| * </Dialog.Body> | |
| * <Dialog.Footer>...</Dialog.Footer> | |
| * </Dialog.Content> | |
| * ``` | |
| */ | |
| Body: DialogBody, | |
| /** | |
| * ### Dialog.Footer | |
| * | |
| * Footer section of the dialog, typically containing action buttons. | |
| * Provides consistent spacing and alignment for dialog actions. | |
| * | |
| * @example | |
| * ```tsx | |
| * <Dialog.Content> | |
| * <Dialog.Header>...</Dialog.Header> | |
| * <Dialog.Body>...</Dialog.Body> | |
| * <Dialog.Footer> | |
| * <Button variant="outline">Cancel</Button> | |
| * <Button>Confirm</Button> | |
| * </Dialog.Footer> | |
| * </Dialog.Content> | |
| * ``` | |
| */ | |
| Footer: DialogFooter, | |
| /** | |
| * ### Dialog.Title | |
| * | |
| * Accessible title element for the dialog. | |
| * Uses React Aria's Heading for proper accessibility and screen reader support. | |
| * | |
| * @example | |
| * ```tsx | |
| * <Dialog.Content> | |
| * <Dialog.Header> | |
| * <Dialog.Title>Confirm Action</Dialog.Title> | |
| * </Dialog.Header> | |
| * <Dialog.Body>...</Dialog.Body> | |
| * </Dialog.Content> | |
| * ``` | |
| */ | |
| Title: DialogTitle, | |
| /** | |
| * ### Dialog.CloseTrigger | |
| * | |
| * Button that closes the dialog when activated. | |
| * Displays an IconButton with a close (X) icon by default. | |
| * | |
| * The component automatically handles the close behavior through React Aria's | |
| * context, so no additional onPress handler is needed. | |
| * | |
| * @example | |
| * ```tsx | |
| * <Dialog.Root> | |
| * <Dialog.Trigger>Open Dialog</Dialog.Trigger> | |
| * <Dialog.Content> | |
| * <Dialog.Header> | |
| * <Dialog.Title>Title</Dialog.Title> | |
| * <Dialog.CloseTrigger aria-label="Close dialog" /> | |
| * </Dialog.Header> | |
| * <Dialog.Body>Content</Dialog.Body> | |
| * </Dialog.Content> | |
| * </Dialog.Root> | |
| * ``` | |
| */ | |
| CloseTrigger: DialogCloseTrigger, | |
| } |
I think that we should follow this convention for commenting every compound component, not just Dialog.
There was a problem hiding this comment.
I agree. I mean I hate it - as it clutters the file - but if the docs are not shown, there is no point in colocating them with the sub-component.
|
@misama-ct I believe this PR needs a changeset |
…ture - Consolidated padding properties in dialog recipe for consistency. - Updated button event handler from onClick to onPress for better accessibility. - Enhanced dialog types by extending slot props and adding ref attributes for improved component integration. - Simplified component implementations by removing unnecessary local refs and merging with forwarded refs. All tests pass successfully, confirming no breaking changes introduced.
…nents - Removed redundant type re-exports from dialog index file for clarity. - Added comprehensive documentation comments for each dialog component, detailing usage and examples. - Streamlined component implementations by removing unnecessary comments and improving readability. All tests pass successfully, confirming no breaking changes introduced.
- Introduced a new i18n file for dialog messages, defining the aria-label for the close trigger button. - Updated DialogCloseTrigger component to utilize internationalized messages for accessibility.
Summary
This PR introduces a complete Dialog component implementation for the Nimbus design system, built with React Aria Components for accessibility and WCAG 2.1 AA compliance.
What's New
🎯 Core Dialog Component
Dialog.Root- Primary container with state managementDialog.Trigger- Button to open the dialogDialog.Content- Main dialog container with sizing and positioningDialog.Header- Header section for title and close buttonDialog.Body- Content area with scroll behavior supportDialog.Footer- Action buttons sectionDialog.Title- Accessible dialog titleDialog.CloseTrigger- Icon button for closing✨ Key Features
🔧 Technical Implementation
📚 Documentation & Testing
🧪 Interactive Test Coverage
Breaking Changes
This implementation replaces the previous Modal-based dialog system with a more comprehensive and accessible solution. Components using the old Dialog API will need to migrate to the new compound component structure.
ARIA Compliance
Test Plan