-
Notifications
You must be signed in to change notification settings - Fork 23
Add Confirmation Dialog System #263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import React from 'react'; | ||
| import { AlertTriangle, Info } from 'lucide-react'; | ||
| import { Button } from './Button'; | ||
| import { Modal } from './Modal'; | ||
| import { ConfirmOptions } from '../../contexts/ConfirmContext'; | ||
|
|
||
| interface ConfirmDialogProps { | ||
| isOpen: boolean; | ||
| onConfirm: () => void; | ||
| onCancel: () => void; | ||
| options: ConfirmOptions; | ||
| } | ||
|
|
||
| export const ConfirmDialog: React.FC<ConfirmDialogProps> = ({ | ||
| isOpen, | ||
| onConfirm, | ||
| onCancel, | ||
| options, | ||
| }) => { | ||
| const { | ||
| title = 'Confirm Action', | ||
| message, | ||
| variant = 'info', | ||
| confirmText = 'Confirm', | ||
| cancelText = 'Cancel', | ||
| } = options; | ||
|
|
||
| const isDanger = variant === 'danger'; | ||
|
|
||
| return ( | ||
| <Modal | ||
| isOpen={isOpen} | ||
| onClose={onCancel} | ||
| title={title} | ||
| footer={ | ||
| <> | ||
| <Button variant="ghost" onClick={onCancel}> | ||
| {cancelText} | ||
| </Button> | ||
| <Button | ||
| variant={isDanger ? 'danger' : 'primary'} | ||
| onClick={onConfirm} | ||
| autoFocus | ||
| > | ||
| {confirmText} | ||
| </Button> | ||
| </> | ||
| } | ||
| > | ||
| <div className="flex items-start gap-4"> | ||
| <div | ||
| className={`p-3 rounded-full flex-shrink-0 ${ | ||
| isDanger ? 'bg-red-100 text-red-600' : 'bg-blue-100 text-blue-600' | ||
| }`} | ||
| > | ||
| {isDanger ? <AlertTriangle size={24} /> : <Info size={24} />} | ||
| </div> | ||
|
Comment on lines
+50
to
+57
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Icon container styling doesn't adapt to themes. The icon background uses hardcoded colors ( Theme-aware icon styling example+import { useTheme } from '../../contexts/ThemeContext';
+import { THEMES } from '../../contexts/ThemeContext';
...
+ const { style } = useTheme();
+
+ const iconContainerStyles = style === THEMES.NEOBRUTALISM
+ ? isDanger
+ ? 'bg-red-500 text-white border-2 border-black'
+ : 'bg-blue-500 text-white border-2 border-black'
+ : isDanger
+ ? 'bg-red-100 text-red-600'
+ : 'bg-blue-100 text-blue-600';
...
<div
- className={`p-3 rounded-full flex-shrink-0 ${
- isDanger ? 'bg-red-100 text-red-600' : 'bg-blue-100 text-blue-600'
- }`}
+ className={`p-3 rounded-full flex-shrink-0 ${iconContainerStyles}`}
>🤖 Prompt for AI Agents |
||
| <div className="mt-1"> | ||
| <p className="text-base opacity-90 leading-relaxed">{message}</p> | ||
| </div> | ||
| </div> | ||
| </Modal> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,64 @@ | ||||||||||||||||||||||||||||||||||||||
| import React, { createContext, useContext, useState, useCallback, ReactNode } from 'react'; | ||||||||||||||||||||||||||||||||||||||
| import { ConfirmDialog } from '../components/ui/ConfirmDialog'; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| export interface ConfirmOptions { | ||||||||||||||||||||||||||||||||||||||
| title?: string; | ||||||||||||||||||||||||||||||||||||||
| message: string; | ||||||||||||||||||||||||||||||||||||||
| variant?: 'info' | 'danger'; | ||||||||||||||||||||||||||||||||||||||
| confirmText?: string; | ||||||||||||||||||||||||||||||||||||||
| cancelText?: string; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| interface ConfirmContextType { | ||||||||||||||||||||||||||||||||||||||
| confirm: (options: ConfirmOptions) => Promise<boolean>; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const ConfirmContext = createContext<ConfirmContextType | undefined>(undefined); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| export const useConfirm = () => { | ||||||||||||||||||||||||||||||||||||||
| const context = useContext(ConfirmContext); | ||||||||||||||||||||||||||||||||||||||
| if (!context) { | ||||||||||||||||||||||||||||||||||||||
| throw new Error('useConfirm must be used within a ConfirmProvider'); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| return context; | ||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| interface ConfirmProviderProps { | ||||||||||||||||||||||||||||||||||||||
| children: ReactNode; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| export const ConfirmProvider: React.FC<ConfirmProviderProps> = ({ children }) => { | ||||||||||||||||||||||||||||||||||||||
| const [isOpen, setIsOpen] = useState(false); | ||||||||||||||||||||||||||||||||||||||
| const [options, setOptions] = useState<ConfirmOptions>({ message: '' }); | ||||||||||||||||||||||||||||||||||||||
| const [resolvePromise, setResolvePromise] = useState<((value: boolean) => void) | null>(null); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const confirm = useCallback((options: ConfirmOptions) => { | ||||||||||||||||||||||||||||||||||||||
| setOptions(options); | ||||||||||||||||||||||||||||||||||||||
| setIsOpen(true); | ||||||||||||||||||||||||||||||||||||||
| return new Promise<boolean>((resolve) => { | ||||||||||||||||||||||||||||||||||||||
| setResolvePromise(() => resolve); | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| }, []); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+35
to
+41
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Concurrent If Suggested fix: reject or resolve previous promise before opening new dialog const confirm = useCallback((options: ConfirmOptions) => {
+ // Resolve any pending promise as cancelled before opening new dialog
+ if (resolvePromise) {
+ resolvePromise(false);
+ }
setOptions(options);
setIsOpen(true);
return new Promise<boolean>((resolve) => {
setResolvePromise(() => resolve);
});
- }, []);
+ }, [resolvePromise]);Alternatively, you could queue confirmations or reject with an error if already open: const confirm = useCallback((options: ConfirmOptions) => {
if (isOpen) {
return Promise.resolve(false); // or throw new Error('Dialog already open')
}
// ... rest of implementation
}, [isOpen]);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const handleConfirm = useCallback(() => { | ||||||||||||||||||||||||||||||||||||||
| if (resolvePromise) resolvePromise(true); | ||||||||||||||||||||||||||||||||||||||
| setIsOpen(false); | ||||||||||||||||||||||||||||||||||||||
| }, [resolvePromise]); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const handleCancel = useCallback(() => { | ||||||||||||||||||||||||||||||||||||||
| if (resolvePromise) resolvePromise(false); | ||||||||||||||||||||||||||||||||||||||
| setIsOpen(false); | ||||||||||||||||||||||||||||||||||||||
| }, [resolvePromise]); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+51
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider clearing After resolving the promise, Suggested cleanup const handleConfirm = useCallback(() => {
if (resolvePromise) resolvePromise(true);
setIsOpen(false);
+ setResolvePromise(null);
}, [resolvePromise]);
const handleCancel = useCallback(() => {
if (resolvePromise) resolvePromise(false);
setIsOpen(false);
+ setResolvePromise(null);
}, [resolvePromise]);🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||
| <ConfirmContext.Provider value={{ confirm }}> | ||||||||||||||||||||||||||||||||||||||
| {children} | ||||||||||||||||||||||||||||||||||||||
| <ConfirmDialog | ||||||||||||||||||||||||||||||||||||||
| isOpen={isOpen} | ||||||||||||||||||||||||||||||||||||||
| onConfirm={handleConfirm} | ||||||||||||||||||||||||||||||||||||||
| onCancel={handleCancel} | ||||||||||||||||||||||||||||||||||||||
| options={options} | ||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||
| </ConfirmContext.Provider> | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing
role="alertdialog"andaria-describedbyfor accessibility.The PR objectives state accessibility support via
role="alertdialog", but this isn't implemented. For alert dialogs, the ARIA role should be set on the dialog container, and focus management should trap focus within the dialog. Additionally, consider placingautoFocuson the cancel button fordangervariants to prevent accidental destructive actions.,
Suggested accessibility improvements
The Modal component should receive ARIA attributes. If Modal doesn't support passing these through, consider wrapping the content or enhancing Modal. At minimum, for alert dialogs:
<Modal isOpen={isOpen} onClose={onCancel} title={title} + role="alertdialog" + aria-describedby="confirm-dialog-message" footer={ <> <Button variant="ghost" onClick={onCancel}> {cancelText} </Button> <Button variant={isDanger ? 'danger' : 'primary'} onClick={onConfirm} - autoFocus + autoFocus={!isDanger} > {confirmText} </Button> </> } >And add the id to the message:
🤖 Prompt for AI Agents