AlertDialog Component #448
Conversation
| Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the addition of a new Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## main #448 +/- ## ======================================= Coverage 94.44% 94.44% ======================================= Files 10 10 Lines 54 54 Branches 9 9 ======================================= Hits 51 51 Misses 3 3 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (14)
src/core/primitives/Floater/index.tsx (2)
3-8: LGTM: Clear and concise Floater object creation.The Floater object is well-structured, providing a clean abstraction over the imported components and hook. This approach enhances usability and allows for easy future extensions.
For consistency, consider using shorthand property names since the property names match the imported names:
const Floater = { - Portal: FloatingPortal, - Overlay: FloatingOverlay, - FocusManager: FloatingFocusManager, - useFloating: useFloating, + Portal, + Overlay, + FocusManager, + useFloating, };
10-10: LGTM: Appropriate default export.The Floater object is correctly exported as the default export, allowing for easy importing in other files.
If you're using TypeScript, consider adding a type definition for the Floater object to improve type safety and developer experience:
import {FloatingOverlay, FloatingPortal, FloatingFocusManager, useFloating} from '@floating-ui/react'; type FloaterType = { Portal: typeof FloatingPortal; Overlay: typeof FloatingOverlay; FocusManager: typeof FloatingFocusManager; useFloating: typeof useFloating; }; const Floater: FloaterType = { Portal, Overlay, FocusManager, useFloating, }; export default Floater;src/components/ui/AlertDialog/shards/AlertDialogPortal.tsx (1)
1-16: Consider adding documentation for the AlertDialogPortal component.The implementation of
AlertDialogPortalis clean and follows React best practices. However, to improve maintainability and ease of use, consider adding documentation:
- Add a brief comment explaining the purpose of this component and how it fits into the larger AlertDialog implementation.
- Include an example of how to use this component.
- If there are any specific considerations or limitations when using this component, mention them in the documentation.
Example:
/** * AlertDialogPortal * * This component is part of the AlertDialog compound component. * It uses Floater.Portal to render its children outside of the parent component's DOM hierarchy, * which is useful for creating modal-like behaviors. * * Usage: * <AlertDialogPortal> * <AlertDialogContent> * {/* Dialog content */} * </AlertDialogContent> * </AlertDialogPortal> */Adding this documentation will make it easier for other developers to understand and use this component correctly.
tsconfig.json (1)
22-22: LGTM! Consider adding a space after the comma for consistency.The addition of "docs" to the
excludearray is a good practice. It prevents the TypeScript compiler from processing documentation files, which is typically unnecessary and can improve compilation speed.For better readability and consistency with common JSON formatting practices, consider adding a space after the comma before "docs":
- "exclude": ["node_modules", "dist", "build","docs"] + "exclude": ["node_modules", "dist", "build", "docs"]src/components/ui/AlertDialog/shards/AlertDialogCancel.tsx (2)
1-3: LGTM! Consider destructuring the React import.The imports look good and appropriate for the component's functionality. A minor suggestion would be to destructure the
useContextimport from React for cleaner code.Consider updating the React import like this:
-import React, {useContext} from 'react'; +import React from 'react'; +import { useContext } from 'react';This separates the default import from the named import, which is a common convention in React projects.
10-19: LGTM! Consider adding a data-testid attribute for easier testing.The
AlertDialogCancelcomponent is well-implemented, using React hooks and context effectively. It's reusable and follows best practices.To improve testability, consider adding a
data-testidattribute to theButtonPrimitive:const AlertDialogCancel = ({children} : AlertDialogCancelProps) => { const {setOpen} = useContext(AlertDialogContext); return ( - <ButtonPrimitive onClick={() => setOpen(false)}> + <ButtonPrimitive onClick={() => setOpen(false)} data-testid="alert-dialog-cancel"> {children} </ButtonPrimitive> ); };This will make it easier to select the component in automated tests.
src/components/ui/AlertDialog/shards/AlertDialogTrigger.tsx (1)
6-8: LGTM: Type definition is clear and exported.The
AlertDialogTriggerPropstype is well-defined and exported, which is good for reusability.Consider if additional props might be needed in the future, such as custom styling options or event handlers. If so, you might want to extend this type to include them.
src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1)
6-18: Good structure, but consider improving type safety and flexibility.The default export follows Storybook's recommended structure and uses SandboxEditor for interactive editing, which is great. However, there are a couple of points to consider:
- Type safety: Instead of using
anyfor args, consider creating a proper type for AlertDialog props.- Flexibility: The content prop is hardcoded. Consider making it part of the args to allow for more flexible stories.
Here's a suggested improvement:
import { ComponentProps } from 'react'; type AlertDialogProps = ComponentProps<typeof AlertDialog>; export default { title: 'UI/Data-Display/AlertDialog', component: AlertDialog, render: ({ content, ...args }: AlertDialogProps) => ( <SandboxEditor> <AlertDialog {...args} content={content} /> </SandboxEditor> ), };This change improves type safety and allows the content to be customized in individual stories.
src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (3)
1-10: LGTM! Minor suggestion for code organization.The imports and type definition look good and appropriate for the component's functionality. The
AlertDialogRootPropstype is well-defined.Consider removing the empty line at line 12 for better code organization.
16-20: LGTM! Consider adding a comment for clarity.The hook usage and state management are well-implemented. Good use of
customClassSwitcherfor flexible styling.Consider adding a brief comment explaining the purpose of
floaterContextand how it's used in the component or its children. This would improve code readability and maintainability.
1-31: Overall, well-implemented component with room for minor enhancements.The
AlertDialogRootcomponent is well-structured and follows React best practices. It effectively sets up a context for managing alert dialog state and uses custom hooks and utilities for enhanced functionality.Consider the following suggestions for further improvement:
- Add JSDoc comments to describe the component's purpose and props, enhancing code documentation.
- Implement basic accessibility features, such as
aria-*attributes, to improve the component's usability for all users.- If not handled elsewhere, consider adding keyboard event listeners for managing dialog focus and dismissal (e.g., Esc key to close).
Would you like assistance in implementing any of these suggestions?
src/components/ui/AlertDialog/AlertDialog.tsx (2)
8-11: LGTM: Well-defined props type. Consider adding JSDoc comments.The
AlertDialogPropstype is well-defined and exported, which is good for reusability. The use ofReact.ReactNodefor bothchildrenandcontentprovides flexibility.Consider adding JSDoc comments to describe the purpose of each prop:
export type AlertDialogProps = { /** The trigger element for the dialog */ children: React.ReactNode; /** The main content to be displayed within the dialog */ content: React.ReactNode; }
14-36: Good overall structure. Consider enhancing flexibility and control.The AlertDialog component is well-structured, using subcomponents logically to create the dialog. The use of Portal for rendering is a good practice for modals.
However, there are a few areas where the component could be improved:
- The Cancel button text is hardcoded, which might be problematic for internationalization or customization.
- There's no way to add additional actions (like a Confirm button) or customize the Cancel button.
- The component doesn't provide a way to control the dialog's open state externally.
Consider the following enhancements:
- Make the Cancel button text customizable:
export type AlertDialogProps = { children: React.ReactNode; content: React.ReactNode; cancelText?: string; } // In the component <AlertDialogCancel> {cancelText || 'Cancel'} </AlertDialogCancel>
- Allow for additional actions:
export type AlertDialogProps = { // ...existing props actions?: React.ReactNode; } // In the component <AlertDialogContent> <div>{content}</div> {actions} <AlertDialogCancel>{cancelText || 'Cancel'}</AlertDialogCancel> </AlertDialogContent>
- Add control for the open state:
export type AlertDialogProps = { // ...existing props isOpen?: boolean; onOpenChange?: (isOpen: boolean) => void; } // In the component <AlertDialogRoot open={isOpen} onOpenChange={onOpenChange}> {/* ...existing content */} </AlertDialogRoot>These changes would make the component more flexible and reusable across different scenarios.
src/components/tools/SandboxEditor/SandboxEditor.tsx (1)
32-32: LGTM! Consider using interface for consistency.The change to make
classNameoptional inSandboxPropsis a good improvement, allowing for more flexible usage of theSandboxEditorcomponent. The implementation correctly handles the optional prop.For consistency with TypeScript best practices, consider using an interface instead of a type alias for
SandboxProps:interface SandboxProps extends PropsWithChildren { className?: string; }This change would make the code more consistent with typical React component prop definitions and potentially more extensible in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
- .eslintignore (1 hunks)
- src/components/tools/SandboxEditor/SandboxEditor.tsx (1 hunks)
- src/components/ui/AlertDialog/AlertDialog.tsx (1 hunks)
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogCancel.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogContent.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogOverlay.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogPortal.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogTrigger.tsx (1 hunks)
- src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1 hunks)
- src/core/primitives/Floater/index.tsx (1 hunks)
- tsconfig.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .eslintignore
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx
🔇 Additional comments (25)
src/core/primitives/Floater/index.tsx (2)
1-1: LGTM: Appropriate imports for floating UI components.The import statement correctly imports the necessary components and hook from '@floating-ui/react', which is a suitable library for creating floating UI elements.
1-10: Overall: Well-structured Floater module with good abstraction.The new Floater module provides a clean abstraction over the floating UI components from '@floating-ui/react'. The code is concise, easy to understand, and follows good practices.
To ensure the new Floater module is being used correctly throughout the codebase, please run the following script:
This script will help identify where and how the new Floater module is being used, allowing you to verify its correct implementation across the project.
✅ Verification successful
Verification Successful: Floater module is correctly integrated throughout the codebase.
The Floater module is properly imported and utilized in the AlertDialog components, ensuring consistent use of the floating UI abstractions.
- Imported in:
AlertDialogOverlay.tsxAlertDialogRoot.tsxAlertDialogPortal.tsx- Components and Hook Usage:
<Floater.Portal><Floater.Overlay>Floater.useFloating()🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new Floater module in the codebase. # Test: Search for imports of the Floater module echo "Searching for imports of the Floater module:" rg --type typescript --type typescriptreact "import .* from ['\"].*Floater['\"]" # Test: Search for usages of Floater components echo "Searching for usages of Floater components:" rg --type typescript --type typescriptreact "<Floater\.(?:Portal|Overlay|FocusManager)" # Test: Search for usages of useFloating hook echo "Searching for usages of useFloating hook:" rg --type typescript --type typescriptreact "Floater\.useFloating"Length of output: 639
Script:
#!/bin/bash # Description: Verify the usage of the new Floater module in the codebase. # Test: Search for imports of the Floater module echo "Searching for imports of the Floater module:" rg --glob '*.ts' --glob '*.tsx' "import .* from ['\"][^'\"]*Floater['\"]" # Test: Search for usages of Floater components echo "Searching for usages of Floater components:" rg --glob '*.ts' --glob '*.tsx' "<Floater\.(?:Portal|Overlay|FocusManager)" # Test: Search for usages of useFloating hook echo "Searching for usages of useFloating hook:" rg --glob '*.ts' --glob '*.tsx' "Floater\.useFloating"Length of output: 1162
src/components/ui/AlertDialog/shards/AlertDialogPortal.tsx (4)
1-2: LGTM: Import statements are correct and necessary.The import statements for React and the custom Floater component are appropriate for this component.
4-6: LGTM: Type definition is clear and properly exported.The
AlertDialogPortalPropstype is well-defined and exported, which is good for reusability. UsingReact.ReactNodefor thechildrenprop is appropriate as it allows any valid React child.
8-14: LGTM: Component implementation is clean and follows best practices.The
AlertDialogPortalcomponent is well-implemented:
- It uses functional component syntax with arrow functions.
- Props are correctly destructured.
- The component properly wraps its children in a
Floater.Portal.The implementation is concise and appropriate for a simple wrapper component.
16-16: LGTM: Export statement is correct.The component is properly exported as the default export, which is appropriate for a single component in a file.
src/components/ui/AlertDialog/shards/AlertDialogOverlay.tsx (4)
1-3: LGTM: Import statements are appropriate and well-organized.The import statements are concise and import only the necessary dependencies. The use of a custom path alias for the Floater import is noted and assumed to be configured correctly in the project setup.
6-17: LGTM: Well-structured component with proper use of context and conditional rendering.The
AlertDialogOverlaycomponent is implemented cleanly, using React hooks and context appropriately. The conditional rendering based on theopenstate is correct, and the use of Tailwind classes for styling the overlay is suitable.
11-13: Verify: Is the empty Floater.Overlay intentional?The
Floater.Overlaycomponent is currently empty. While this might be intentional if it's purely for visual overlay purposes, it's worth confirming that no content is meant to be placed inside it.Could you please confirm if the empty
Floater.Overlayis intentional, or if there should be any content or child components within it?
19-19: LGTM: Correct export statement.The default export of the
AlertDialogOverlaycomponent is appropriate and follows common practices for React components.src/components/ui/AlertDialog/shards/AlertDialogCancel.tsx (2)
6-8: LGTM! Type definition is clear and appropriate.The
AlertDialogCancelPropstype is well-defined and exported, allowing for proper typing of the component's props and potential reuse in other files.
1-19: Great job on implementing the AlertDialogCancel component!The implementation is clean, well-structured, and follows React best practices. It effectively uses context for state management and provides a reusable component for canceling alert dialogs. The code is easy to read and maintain, which aligns well with the PR objectives.
src/components/ui/AlertDialog/shards/AlertDialogContent.tsx (4)
1-2: LGTM: Imports are appropriate and well-organized.The imports are correctly structured, importing only the necessary items from React and using a local import for the context. This demonstrates good code organization and separation of concerns.
5-7: LGTM: Type definition is clear and properly exported.The
AlertDialogContentPropstype is well-defined, usingReact.ReactNodefor thechildrenprop, which provides flexibility for the component's content. Exporting the type is a good practice for reusability.
9-21: LGTM: Component structure and logic are well-implemented.The
AlertDialogContentcomponent is well-structured, using React hooks appropriately and implementing conditional rendering based on the context state. The use of Tailwind CSS classes for styling is consistent with modern practices.
23-23: LGTM: Appropriate use of default export.The component is correctly exported as the default, which is suitable for a single component in a file and aligns with common React practices.
src/components/ui/AlertDialog/shards/AlertDialogTrigger.tsx (3)
1-4: LGTM: Imports are correct and necessary.The imports are appropriate for the component's functionality. The use of the
~alias in the ButtonPrimitive import path is noted and assumed to be correctly configured in the project's build setup.
21-21: LGTM: Export statement is correct.The default export of the AlertDialogTrigger component is appropriate and follows common React component export patterns.
1-21: Overall assessment: Good implementation with minor improvements needed.The AlertDialogTrigger component is well-structured and serves its purpose effectively. It correctly integrates with the AlertDialogContext for state management and provides a reusable trigger for alert dialogs.
Key points:
- Imports and type definitions are clear and appropriate.
- The component logic is sound, using context to manage dialog state.
- Minor improvements are suggested for removing unused context and handling prop spreading more safely.
Once the suggested changes are implemented, this component will be a robust and efficient part of the alert dialog system.
src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1)
1-4: LGTM: Imports are appropriate for a Storybook story file.The imports include React, the AlertDialog component, and a SandboxEditor component, which are all relevant for creating a Storybook story.
src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (3)
13-15: LGTM! Well-structured component definition.The constant
COMPONENT_NAMEis clearly defined, and theAlertDialogRootfunction signature is well-structured. Good use of prop destructuring and default value assignment forcustomRootClass.
21-28: LGTM! Clean and clear render logic.The component's render logic is well-structured. Good use of
AlertDialogContext.Providerto provide context to child components. The rendering of thedivwithrootClassandchildrenis clean and allows for flexible styling.
30-31: LGTM! Proper component export and naming.Setting the
displayNameis good practice for debugging. The default export is appropriate for this component.src/components/ui/AlertDialog/AlertDialog.tsx (2)
1-7: LGTM: Imports are well-organized and appropriate.The imports are correctly structured, importing React and all necessary subcomponents for the AlertDialog. The naming convention is consistent, which aids in code readability.
38-38: LGTM: Appropriate default export.The default export of the AlertDialog component is correct and allows for easy import in other files.
| <div className="alert-content fixed left-1/2 top-1/2 z-50 -translate-x-1/2 -translate-y-1/2 bg-gray-900/50"> | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing accessibility for the dialog content.
While the component structure is good, it lacks some important accessibility features for a dialog. Consider the following improvements:
- Add appropriate ARIA attributes such as
role="dialog"andaria-modal="true". - Implement focus management when the dialog opens and closes.
- Add a mechanism to close the dialog when pressing the Escape key.
Here's a suggested implementation:
const AlertDialogContent = ({children} : AlertDialogContentProps) => { - const {open} = useContext(AlertDialogContext); + const {open, onClose} = useContext(AlertDialogContext); + const dialogRef = React.useRef<HTMLDivElement>(null); + + React.useEffect(() => { + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + onClose(); + } + }; + + if (open) { + document.addEventListener('keydown', handleKeyDown); + dialogRef.current?.focus(); + } + + return () => { + document.removeEventListener('keydown', handleKeyDown); + }; + }, [open, onClose]); return ( <> {open && ( - <div className="alert-content fixed left-1/2 top-1/2 z-50 -translate-x-1/2 -translate-y-1/2 bg-gray-900/50"> + <div + ref={dialogRef} + role="dialog" + aria-modal="true" + className="alert-content fixed left-1/2 top-1/2 z-50 -translate-x-1/2 -translate-y-1/2 bg-gray-900/50" + tabIndex={-1} + > {children} </div> )} </> ); };This implementation adds keyboard support, focus management, and proper ARIA attributes for better accessibility.
Committable suggestion was skipped due to low confidence.
| const AlertDialogTrigger = ({children, ...props} : AlertDialogTriggerProps) => { | ||
| const {floaterContext} = useContext(AlertDialogContext); | ||
| const {open, setOpen} = useContext(AlertDialogContext); | ||
| | ||
| return ( | ||
| <ButtonPrimitive onClick={() => setOpen(true)} {...props}> | ||
| {children} | ||
| </ButtonPrimitive> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Remove unused context and ensure prop spreading doesn't override onClick.
The component implementation is generally good, but there are two issues to address:
- The
floaterContextis extracted from the context but never used. This should be removed. - Spreading props onto ButtonPrimitive after defining onClick could potentially override the onClick handler if props include their own onClick.
Here's a suggested fix:
-const AlertDialogTrigger = ({children, ...props} : AlertDialogTriggerProps) => { - const {floaterContext} = useContext(AlertDialogContext); - const {open, setOpen} = useContext(AlertDialogContext); +const AlertDialogTrigger = ({children, onClick, ...props} : AlertDialogTriggerProps) => { + const {setOpen} = useContext(AlertDialogContext); return ( - <ButtonPrimitive onClick={() => setOpen(true)} {...props}> + <ButtonPrimitive + {...props} + onClick={(e) => { + setOpen(true); + onClick?.(e); + }} + > {children} </ButtonPrimitive> ); };This change removes the unused context, ensures that a custom onClick prop won't override the dialog opening functionality, and allows for additional onClick behavior if provided.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const AlertDialogTrigger = ({children, ...props} : AlertDialogTriggerProps) => { | |
| const {floaterContext} = useContext(AlertDialogContext); | |
| const {open, setOpen} = useContext(AlertDialogContext); | |
| return ( | |
| <ButtonPrimitive onClick={() => setOpen(true)} {...props}> | |
| {children} | |
| </ButtonPrimitive> | |
| ); | |
| }; | |
| const AlertDialogTrigger = ({children, onClick, ...props} : AlertDialogTriggerProps) => { | |
| const {setOpen} = useContext(AlertDialogContext); | |
| return ( | |
| <ButtonPrimitive | |
| {...props} | |
| onClick={(e) => { | |
| setOpen(true); | |
| onClick?.(e); | |
| }} | |
| > | |
| {children} | |
| </ButtonPrimitive> | |
| ); | |
| }; |
| // More on writing stories with args: https://storybook.js.org/docs/react/writing-stories/args | ||
| export const Default = { | ||
| args: { | ||
| children: 'This is trigger', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider showcasing more props and functionality in the Default story.
The Default story provides a basic example, but it could be enhanced to demonstrate more of the AlertDialog's capabilities:
- Include more props that AlertDialog accepts, if any (e.g., isOpen, onClose).
- Demonstrate how the 'content' prop can be customized.
- Consider adding more stories to showcase different use cases or configurations.
Here's a suggested improvement:
export const Default = { args: { children: <button>Open Alert Dialog</button>, content: ( <div className="flex flex-col gap-4 text-gray-100"> <h2>Alert Title</h2> <p>This is the alert content.</p> <button>Close</button> </div> ), isOpen: false, onClose: () => console.log('Dialog closed'), }, }; export const OpenByDefault = { ...Default, args: { ...Default.args, isOpen: true, }, };This change provides a more comprehensive demonstration of the AlertDialog component and adds an additional story variant.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/ui/AlertDialog/AlertDialog.tsx (2)
8-11: LGTM: Type definition is clear and flexible. Consider adding JSDoc comments.The
AlertDialogPropstype is well-defined and exported, allowing for reuse. The use ofReact.ReactNodefor both props provides flexibility.Consider adding JSDoc comments to describe the purpose of each prop:
export type AlertDialogProps = { /** The trigger element for the dialog */ children: React.ReactNode; /** The main content to be displayed within the dialog */ content: React.ReactNode; }
13-37: LGTM: Well-structured component. Consider enhancing flexibility and internationalization support.The
AlertDialogcomponent is well-structured and uses composition effectively. However, there are a few areas where it could be improved:
- The "Cancel" text is hardcoded, which may not be suitable for all use cases or languages.
- There's no way to customize the cancel button or add additional actions.
- The component doesn't provide a way to handle the dialog's open/close state externally.
Consider the following enhancements:
- Accept a
cancelTextprop to allow customization of the cancel button text:export type AlertDialogProps = { children: React.ReactNode; content: React.ReactNode; cancelText?: string; } const AlertDialog = ({ children, content, cancelText = 'Cancel' }: AlertDialogProps) => { // ... <AlertDialogCancel> {cancelText} </AlertDialogCancel> // ... }
- Add an
actionsprop to allow for additional buttons or actions:export type AlertDialogProps = { // ... actions?: React.ReactNode; } const AlertDialog = ({ children, content, cancelText = 'Cancel', actions }: AlertDialogProps) => { // ... <AlertDialogContent> <div>{content}</div> <div> {actions} <AlertDialogCancel>{cancelText}</AlertDialogCancel> </div> </AlertDialogContent> // ... }
- Consider adding an
openprop and anonOpenChangecallback to allow external control of the dialog's state:export type AlertDialogProps = { // ... open?: boolean; onOpenChange?: (open: boolean) => void; } const AlertDialog = ({ children, content, cancelText = 'Cancel', actions, open, onOpenChange }: AlertDialogProps) => { return ( <AlertDialogRoot open={open} onOpenChange={onOpenChange}> {/* ... */} </AlertDialogRoot> ); }These changes would make the component more flexible and easier to use in various scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
- src/components/tools/SandboxEditor/SandboxEditor.tsx (1 hunks)
- src/components/ui/AlertDialog/AlertDialog.tsx (1 hunks)
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogCancel.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogContent.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogOverlay.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogPortal.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogTrigger.tsx (1 hunks)
- src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1 hunks)
- src/core/primitives/Floater/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/components/tools/SandboxEditor/SandboxEditor.tsx
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx
- src/components/ui/AlertDialog/shards/AlertDialogCancel.tsx
- src/components/ui/AlertDialog/shards/AlertDialogContent.tsx
- src/components/ui/AlertDialog/shards/AlertDialogOverlay.tsx
- src/components/ui/AlertDialog/shards/AlertDialogPortal.tsx
- src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx
- src/components/ui/AlertDialog/shards/AlertDialogTrigger.tsx
- src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx
- src/core/primitives/Floater/index.tsx
🔇 Additional comments (1)
src/components/ui/AlertDialog/AlertDialog.tsx (1)
1-7: LGTM: Imports are well-organized and follow best practices.The imports are appropriately structured, promoting modularity by separating subcomponents into individual files. The naming convention is consistent, which enhances readability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (2)
1-14: LGTM! Consider grouping imports for better organization.The imports and type definitions look good. The
AlertDialogRootPropstype is well-defined with clear prop types.Consider grouping the imports for better organization:
import React, { useState } from 'react'; import { customClassSwitcher } from '~/core'; import Floater from '~/core/primitives/Floater'; import { AlertDialogContext } from '../contexts/AlertDialogContext'; // ... rest of the code
16-34: LGTM! Consider memoizing the context value for potential performance gains.The component implementation looks good. It correctly manages state, provides context, and allows for flexible styling.
For potential performance optimization, consider memoizing the context value:
import React, { useState, useMemo } from 'react'; // ... other imports const AlertDialogRoot = ({ children, customRootClass = '', open, onOpenChange } : AlertDialogRootProps) => { // ... existing code const contextValue = useMemo(() => ({ isOpen, handleOpenChange, floaterContext }), [isOpen, floaterContext]); return ( <AlertDialogContext.Provider value={contextValue}> <div className={rootClass}> {children} </div> </AlertDialogContext.Provider> ); };This change can help prevent unnecessary re-renders of child components that consume the context.
src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1)
1-38: Overall assessment: Good foundation, but room for improvementThe AlertDialog story provides a basic structure following Storybook conventions. However, there are several opportunities to enhance its effectiveness:
- The default export could be more flexible, allowing for better control and demonstration of the AlertDialog's props.
- The Default story could be expanded to showcase more of the component's capabilities.
- Additional story variants could be added to demonstrate different use cases or configurations.
Implementing the suggested improvements in the previous comments would significantly enhance the quality and usefulness of this Storybook story, providing better documentation and testing capabilities for the AlertDialog component.
Consider adding more comprehensive documentation within the story file, such as JSDoc comments for the component and its props. This would improve the story's value as both a visual demonstration and a source of component documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- src/components/ui/AlertDialog/AlertDialog.tsx (1 hunks)
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogCancel.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogContent.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogOverlay.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogTrigger.tsx (1 hunks)
- src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx
- src/components/ui/AlertDialog/shards/AlertDialogCancel.tsx
- src/components/ui/AlertDialog/shards/AlertDialogContent.tsx
- src/components/ui/AlertDialog/shards/AlertDialogOverlay.tsx
- src/components/ui/AlertDialog/shards/AlertDialogTrigger.tsx
🔇 Additional comments (6)
src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (2)
36-37: LGTM! Proper display name and export.The display name is correctly set using the COMPONENT_NAME constant, which is good for debugging. The default export is appropriate for this component.
1-37: Overall, well-implemented AlertDialogRoot component.The AlertDialogRoot component is well-structured and provides a solid foundation for an alert dialog with proper state management and context provision. The minor suggestions for import organization and context value memoization can further enhance the code quality and performance. Great job on this implementation!
src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (2)
1-4: LGTM: Imports are appropriate and well-structured.The imports cover all necessary dependencies for the story, including React hooks and custom components. The use of path aliases (starting with '~') is a good practice for managing imports in larger projects.
33-38: 🛠️ Refactor suggestionEnhance the Default story to showcase more props and functionality.
The current Default story is minimal and doesn't fully demonstrate the capabilities of the AlertDialog component. As suggested in the previous review, consider expanding this story to provide a more comprehensive example.
Here's a suggested improvement:
export const Default = { args: { children: <button>Open Alert Dialog</button>, content: ( <div className="flex flex-col gap-4 text-gray-100"> <h2>Alert Title</h2> <p>This is the alert content.</p> <button>Close</button> </div> ), open: false, }, }; export const OpenByDefault = { ...Default, args: { ...Default.args, open: true, }, };This change:
- Demonstrates more props that AlertDialog accepts.
- Shows how the 'content' prop can be customized.
- Adds an additional story variant to showcase different configurations.
src/components/ui/AlertDialog/AlertDialog.tsx (2)
1-7: LGTM: Imports are well-organized and follow modular design principles.The imports are appropriate for the component's functionality. Separating subcomponents into individual files promotes modularity and maintainability.
38-38: LGTM: Component export is appropriate.The default export of the AlertDialog component is correct and follows common React component export patterns.
| // More on how to set up stories at: https://storybook.js.org/docs/react/writing-stories/introduction#default-export | ||
| export default { | ||
| title: 'UI/Data-Display/AlertDialog', | ||
| component: AlertDialog, | ||
| render: (args:any) => { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| | ||
| const handleOpenChange = (open: boolean) => { | ||
| console.log('open', open); | ||
| setIsOpen(open); | ||
| }; | ||
| | ||
| return ( | ||
| <SandboxEditor> | ||
| <AlertDialog | ||
| open={isOpen} | ||
| onOpenChange={handleOpenChange} | ||
| {...args} content={ | ||
| <div className="flex flex-col gap-4 text-gray-100"> | ||
| <h1>This is content</h1> | ||
| </div> | ||
| } /> | ||
| </SandboxEditor> | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the story configuration for better flexibility and demonstration.
While the current setup is functional, there are a few areas that could be improved:
- The
contentprop is hardcoded, which limits the story's flexibility. Consider making it configurable through story args. - The
console.loginhandleOpenChangemight not be necessary in production. Consider using a more robust logging mechanism or removing it if it's not needed. - The story could benefit from showcasing more props and functionality of the AlertDialog component.
Here's a suggested improvement:
export default { title: 'UI/Data-Display/AlertDialog', component: AlertDialog, argTypes: { content: { control: 'text' }, open: { control: 'boolean' }, }, render: ({ content, open: initialOpen, ...args }: any) => { const [isOpen, setIsOpen] = useState(initialOpen); const handleOpenChange = (open: boolean) => { setIsOpen(open); }; return ( <SandboxEditor> <AlertDialog open={isOpen} onOpenChange={handleOpenChange} content={ <div className="flex flex-col gap-4 text-gray-100"> {content || <h1>This is content</h1>} </div> } {...args} /> </SandboxEditor> ); } };This change allows for more dynamic content and better control over the open state in Storybook.
| export type AlertDialogProps = { | ||
| children: React.ReactNode; | ||
| content: React.ReactNode; | ||
| } |
There was a problem hiding this comment.
Add missing props to AlertDialogProps type definition.
The AlertDialogProps type is missing the 'open' and 'onOpenChange' props that are used in the component. Please update the type definition to include these props:
Apply this diff to add the missing props:
export type AlertDialogProps = { children: React.ReactNode; content: React.ReactNode; + open?: boolean; + onOpenChange?: (open: boolean) => void; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type AlertDialogProps = { | |
| children: React.ReactNode; | |
| content: React.ReactNode; | |
| } | |
| export type AlertDialogProps = { | |
| children: React.ReactNode; | |
| content: React.ReactNode; | |
| open?: boolean; | |
| onOpenChange?: (open: boolean) => void; | |
| } |
| const AlertDialog = ({ children, open, onOpenChange, content } : AlertDialogProps) => { | ||
| const [isOpen, setIsOpen] = useState(open); | ||
| return ( | ||
| <AlertDialogRoot open={isOpen} onOpenChange={onOpenChange}> | ||
| <AlertDialogTrigger> | ||
| {children} | ||
| </AlertDialogTrigger> | ||
| | ||
| <AlertDialogPortal> | ||
| <AlertDialogOverlay/> | ||
| <AlertDialogContent> | ||
| <div> | ||
| {content} | ||
| </div> | ||
| <AlertDialogCancel> | ||
| Cancel | ||
| </AlertDialogCancel> | ||
| </AlertDialogContent> | ||
| | ||
| </AlertDialogPortal> | ||
| | ||
| </AlertDialogRoot> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Refactor component to improve prop usage and add proper TypeScript typing.
The current implementation has a few issues:
- The useState hook for managing open state is redundant as it's already provided as a prop.
- The isOpen state is not being used to control the AlertDialogRoot's open prop.
- The onOpenChange prop is not being used effectively.
- The component lacks proper TypeScript typing for its props.
Please refactor the component as follows:
-const AlertDialog = ({ children, open, onOpenChange, content } : AlertDialogProps) => { - const [isOpen, setIsOpen] = useState(open); +const AlertDialog: React.FC<AlertDialogProps> = ({ children, open, onOpenChange, content }) => { return ( - <AlertDialogRoot open={isOpen} onOpenChange={onOpenChange}> + <AlertDialogRoot open={open} onOpenChange={onOpenChange}> <AlertDialogTrigger> {children} </AlertDialogTrigger> <AlertDialogPortal> <AlertDialogOverlay/> <AlertDialogContent> <div> {content} </div> <AlertDialogCancel> Cancel </AlertDialogCancel> </AlertDialogContent> </AlertDialogPortal> </AlertDialogRoot> ); };This refactoring removes the redundant state management, properly uses the open prop, and adds correct TypeScript typing to the component.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const AlertDialog = ({ children, open, onOpenChange, content } : AlertDialogProps) => { | |
| const [isOpen, setIsOpen] = useState(open); | |
| return ( | |
| <AlertDialogRoot open={isOpen} onOpenChange={onOpenChange}> | |
| <AlertDialogTrigger> | |
| {children} | |
| </AlertDialogTrigger> | |
| <AlertDialogPortal> | |
| <AlertDialogOverlay/> | |
| <AlertDialogContent> | |
| <div> | |
| {content} | |
| </div> | |
| <AlertDialogCancel> | |
| Cancel | |
| </AlertDialogCancel> | |
| </AlertDialogContent> | |
| </AlertDialogPortal> | |
| </AlertDialogRoot> | |
| ); | |
| }; | |
| const AlertDialog: React.FC<AlertDialogProps> = ({ children, open, onOpenChange, content }) => { | |
| return ( | |
| <AlertDialogRoot open={open} onOpenChange={onOpenChange}> | |
| <AlertDialogTrigger> | |
| {children} | |
| </AlertDialogTrigger> | |
| <AlertDialogPortal> | |
| <AlertDialogOverlay/> | |
| <AlertDialogContent> | |
| <div> | |
| {content} | |
| </div> | |
| <AlertDialogCancel> | |
| Cancel | |
| </AlertDialogCancel> | |
| </AlertDialogContent> | |
| </AlertDialogPortal> | |
| </AlertDialogRoot> | |
| ); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
styles/themes/components/alert-dialog.scss (1)
1-23: Enhance accessibility and user experience of the AlertDialog component.While the basic styles for the AlertDialog are in place, there are some improvements we can make for better accessibility and user experience:
Accessibility:
- Add
aria-modal="true"to the dialog content in the React component.- Ensure focus is trapped within the dialog when open.
- Add a visible focus outline for keyboard navigation.
User Experience:
- Add transitions for a smoother open/close experience.
Consider these additions to your styles:
.rad-ui-alert-dialog-overlay { /* ... existing styles ... */ opacity: 0; transition: opacity 0.2s ease-in-out; &[data-state="open"] { opacity: 1; } } .rad-ui-alert-dialog-content { /* ... existing styles ... */ opacity: 0; transform: translate(-50%, -48%) scale(0.96); transition: opacity 0.2s ease-in-out, transform 0.2s ease-in-out; &[data-state="open"] { opacity: 1; transform: translate(-50%, -50%) scale(1); } &:focus { outline: 2px solid blue; outline-offset: 2px; } }These changes will add smooth transitions when opening/closing the dialog and improve keyboard accessibility. Remember to implement the corresponding React component logic to manage the
data-stateattribute and focus management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/components/ui/AlertDialog/shards/AlertDialogContent.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogOverlay.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (1 hunks)
- src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1 hunks)
- styles/themes/components/alert-dialog.scss (1 hunks)
- styles/themes/default.scss (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/ui/AlertDialog/shards/AlertDialogContent.tsx
- src/components/ui/AlertDialog/shards/AlertDialogOverlay.tsx
- src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx
- src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx
🔇 Additional comments (1)
styles/themes/default.scss (1)
2-2: LGTM! Verify AlertDialog component implementation.The import for
alert-dialog.scsshas been correctly added and follows the existing pattern of component imports. This suggests that a new AlertDialog component has been created.To ensure the AlertDialog component has been properly implemented, please run the following script:
This script will help verify that the necessary files for the AlertDialog component exist and that the component is properly exported.
✅ Verification successful
Please run the updated verification script to ensure the AlertDialog component is properly exported:
Please run the updated verification script to ensure the AlertDialog component exports are correctly identified:
AlertDialog Component Verified
The
alert-dialog.scssfile and all associated AlertDialog component files are present and correctly exported within thesrc/componentsdirectory. The implementation follows the established patterns and integrates seamlessly with the existing styling and component structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and basic structure of the AlertDialog component # Check if the alert-dialog.scss file exists if fd -t f "alert-dialog.scss" src/components; then echo "alert-dialog.scss file found." else echo "alert-dialog.scss file not found in the components directory." fi # Check for the existence of an AlertDialog component file (e.g., AlertDialog.tsx or AlertDialog.js) if fd -t f "AlertDialog.(tsx|js)" src/components; then echo "AlertDialog component file found." else echo "AlertDialog component file not found in the components directory." fi # Search for exports of an AlertDialog component rg -t typescript -t javascript "export.*AlertDialog" src/componentsLength of output: 387
Script:
#!/bin/bash # Description: Verify the exports of the AlertDialog component # Search for exports of the AlertDialog component in TypeScript and JavaScript files rg "export.*AlertDialog" src/components --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' --type tsx --type tsLength of output: 166
Script:
#!/bin/bash # Description: Verify the exports of the AlertDialog component # Search for exports of the AlertDialog component in TypeScript and JavaScript files rg "export\s+.*AlertDialog" src/components --glob '*.ts' --glob '*.tsx'Length of output: 1388
| .rad-ui-alert-dialog-content { | ||
| padding: 16px; | ||
| border-radius: 8px; | ||
| background-color: white; | ||
| color: black; | ||
| width: 100%; | ||
| height: 100%; | ||
| max-width: 400px; | ||
| max-height: 400px; | ||
| height: 100%; | ||
| margin: 10px auto; | ||
| position: fixed; | ||
| left: 50%; | ||
| top: 50%; | ||
| transform: translate(-50%, -50%); | ||
| z-index: 50; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine dialog content styles for better consistency and responsiveness.
The dialog content styles are generally good, but there are a few issues to address:
- There's a duplicate
height: 100%;property (lines 13 and 16). - Setting both
width: 100%;andmax-width: 400px;(and similarly for height) may lead to unexpected behavior on smaller screens.
Consider applying these changes:
.rad-ui-alert-dialog-content { padding: 16px; border-radius: 8px; background-color: white; color: black; - width: 100%; - height: 100%; max-width: 400px; max-height: 400px; - height: 100%; + width: 90%; /* Use percentage for responsiveness */ + height: auto; /* Allow content to determine height */ margin: 10px auto; position: fixed; left: 50%; top: 50%; transform: translate(-50%, -50%); z-index: 50; + overflow-y: auto; /* Add scrolling for overflow content */ }These changes will:
- Remove the duplicate height property
- Make the dialog more responsive on different screen sizes
- Allow the content to determine the height, with scrolling if needed
Additionally, consider adding media queries to adjust the dialog size on smaller screens:
@media (max-width: 480px) { .rad-ui-alert-dialog-content { width: 95%; max-width: none; } }This will ensure the dialog is usable on mobile devices.
| .rad-ui-alert-dialog-overlay { | ||
| z-index: 50; | ||
| background-color: rgba(0, 0, 0, 0.5); | ||
| | ||
| } |
There was a problem hiding this comment.
Consider adding positioning and size properties to the overlay.
The overlay class looks good overall, but it's missing some crucial properties:
- The overlay should cover the entire viewport.
- It should have a fixed position to stay in place when scrolling.
Consider adding these properties to ensure the overlay behaves correctly:
.rad-ui-alert-dialog-overlay { z-index: 50; background-color: rgba(0, 0, 0, 0.5); - + position: fixed; + top: 0; + left: 0; + right: 0; + bottom: 0; }This will ensure the overlay covers the entire screen and stays in place when scrolling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .rad-ui-alert-dialog-overlay { | |
| z-index: 50; | |
| background-color: rgba(0, 0, 0, 0.5); | |
| } | |
| .rad-ui-alert-dialog-overlay { | |
| z-index: 50; | |
| background-color: rgba(0, 0, 0, 0.5); | |
| position: fixed; | |
| top: 0; | |
| left: 0; | |
| right: 0; | |
| bottom: 0; | |
| } |
This PR adds a new AlertDialog component - #20
Summary by CodeRabbit
Release Notes
New Features
AlertDialogcomponent, providing a customizable alert dialog interface.AlertDialogTrigger,AlertDialogContent,AlertDialogCancel,AlertDialogAction,AlertDialogOverlay, andAlertDialogRootfor enhanced dialog functionality.Improvements
SandboxEditorto allow optionalclassNameprop, enhancing flexibility.Styles
Documentation
AlertDialogcomponent to facilitate testing and visualization.