-
Notifications
You must be signed in to change notification settings - Fork 24
[UX] Confirmation Dialog System #256
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
base: main
Are you sure you want to change the base?
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,94 @@ | ||||||||||||||||||||||||||||||||||||||||
| import { AlertTriangle, Info } from 'lucide-react'; | ||||||||||||||||||||||||||||||||||||||||
| import React from 'react'; | ||||||||||||||||||||||||||||||||||||||||
| import { THEMES } from '../../constants'; | ||||||||||||||||||||||||||||||||||||||||
| import { useTheme } from '../../contexts/ThemeContext'; | ||||||||||||||||||||||||||||||||||||||||
| import { Button } from './Button'; | ||||||||||||||||||||||||||||||||||||||||
| import { Modal } from './Modal'; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| export type ConfirmVariant = 'danger' | 'warning' | 'info'; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| export interface ConfirmDialogProps { | ||||||||||||||||||||||||||||||||||||||||
| isOpen: boolean; | ||||||||||||||||||||||||||||||||||||||||
| title: string; | ||||||||||||||||||||||||||||||||||||||||
| description: string; | ||||||||||||||||||||||||||||||||||||||||
| confirmText?: string; | ||||||||||||||||||||||||||||||||||||||||
| cancelText?: string; | ||||||||||||||||||||||||||||||||||||||||
| variant?: ConfirmVariant; | ||||||||||||||||||||||||||||||||||||||||
| onConfirm: () => void; | ||||||||||||||||||||||||||||||||||||||||
| onCancel: () => void; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| export const ConfirmDialog: React.FC<ConfirmDialogProps> = ({ | ||||||||||||||||||||||||||||||||||||||||
| isOpen, | ||||||||||||||||||||||||||||||||||||||||
| title, | ||||||||||||||||||||||||||||||||||||||||
| description, | ||||||||||||||||||||||||||||||||||||||||
| confirmText = 'Confirm', | ||||||||||||||||||||||||||||||||||||||||
| cancelText = 'Cancel', | ||||||||||||||||||||||||||||||||||||||||
| variant = 'danger', | ||||||||||||||||||||||||||||||||||||||||
| onConfirm, | ||||||||||||||||||||||||||||||||||||||||
| onCancel, | ||||||||||||||||||||||||||||||||||||||||
| }) => { | ||||||||||||||||||||||||||||||||||||||||
| const { style, mode } = useTheme(); | ||||||||||||||||||||||||||||||||||||||||
| const isNeo = style === THEMES.NEOBRUTALISM; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Determine styles based on variant | ||||||||||||||||||||||||||||||||||||||||
| const getIcon = () => { | ||||||||||||||||||||||||||||||||||||||||
| switch (variant) { | ||||||||||||||||||||||||||||||||||||||||
| case 'danger': | ||||||||||||||||||||||||||||||||||||||||
| return <AlertTriangle size={32} className={isNeo ? 'text-black' : 'text-red-500'} />; | ||||||||||||||||||||||||||||||||||||||||
| case 'warning': | ||||||||||||||||||||||||||||||||||||||||
| return <AlertTriangle size={32} className={isNeo ? 'text-black' : 'text-yellow-500'} />; | ||||||||||||||||||||||||||||||||||||||||
| case 'info': | ||||||||||||||||||||||||||||||||||||||||
| return <Info size={32} className={isNeo ? 'text-black' : 'text-blue-500'} />; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const getIconBg = () => { | ||||||||||||||||||||||||||||||||||||||||
| switch (variant) { | ||||||||||||||||||||||||||||||||||||||||
| case 'danger': | ||||||||||||||||||||||||||||||||||||||||
| return isNeo ? 'bg-red-400 border-2 border-black rounded-none' : 'bg-red-500/20 rounded-full'; | ||||||||||||||||||||||||||||||||||||||||
| case 'warning': | ||||||||||||||||||||||||||||||||||||||||
| return isNeo ? 'bg-yellow-400 border-2 border-black rounded-none' : 'bg-yellow-500/20 rounded-full'; | ||||||||||||||||||||||||||||||||||||||||
| case 'info': | ||||||||||||||||||||||||||||||||||||||||
| return isNeo ? 'bg-blue-400 border-2 border-black rounded-none' : 'bg-blue-500/20 rounded-full'; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const getButtonVariant = () => { | ||||||||||||||||||||||||||||||||||||||||
| switch (variant) { | ||||||||||||||||||||||||||||||||||||||||
| case 'danger': return 'danger'; | ||||||||||||||||||||||||||||||||||||||||
| case 'warning': return 'primary'; | ||||||||||||||||||||||||||||||||||||||||
| case 'info': return 'primary'; | ||||||||||||||||||||||||||||||||||||||||
| default: return 'primary'; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||
| <Modal | ||||||||||||||||||||||||||||||||||||||||
| isOpen={isOpen} | ||||||||||||||||||||||||||||||||||||||||
| onClose={onCancel} | ||||||||||||||||||||||||||||||||||||||||
| title={title} | ||||||||||||||||||||||||||||||||||||||||
| footer={ | ||||||||||||||||||||||||||||||||||||||||
| <> | ||||||||||||||||||||||||||||||||||||||||
| <Button variant="ghost" onClick={onCancel}> | ||||||||||||||||||||||||||||||||||||||||
| {cancelText} | ||||||||||||||||||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||||||||||||||||||
| <Button variant={getButtonVariant()} onClick={onConfirm} autoFocus> | ||||||||||||||||||||||||||||||||||||||||
| {confirmText} | ||||||||||||||||||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||||||||||||||||||
| </> | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+71
to
+79
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. Consider focusing the Cancel button for destructive actions. The 🛡️ Suggested fix footer={
<>
- <Button variant="ghost" onClick={onCancel}>
+ <Button variant="ghost" onClick={onCancel} autoFocus={variant === 'danger' || variant === 'warning'}>
{cancelText}
</Button>
- <Button variant={getButtonVariant()} onClick={onConfirm} autoFocus>
+ <Button variant={getButtonVariant()} onClick={onConfirm} autoFocus={variant === 'info'}>
{confirmText}
</Button>
</>
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||||||||||||
| <div className="flex flex-col items-center text-center sm:flex-row sm:text-left sm:items-start gap-4"> | ||||||||||||||||||||||||||||||||||||||||
| <div className={`p-3 shrink-0 ${getIconBg()}`}> | ||||||||||||||||||||||||||||||||||||||||
| {getIcon()} | ||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||
| <div> | ||||||||||||||||||||||||||||||||||||||||
| <p className={`text-base leading-relaxed ${isNeo ? 'text-black' : 'text-white/80'}`}> | ||||||||||||||||||||||||||||||||||||||||
| {description} | ||||||||||||||||||||||||||||||||||||||||
| </p> | ||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||
| </Modal> | ||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,7 +43,7 @@ export const Modal: React.FC<ModalProps> = ({ isOpen, onClose, title, children, | |||||||||||||
| return ( | ||||||||||||||
| <AnimatePresence> | ||||||||||||||
| {isOpen && ( | ||||||||||||||
| <div className="fixed inset-0 z-50 flex items-center justify-center p-4"> | ||||||||||||||
| <div className="fixed inset-0 z-50 flex items-center justify-center p-4" role="dialog" aria-modal="true" aria-labelledby="modal-title"> | ||||||||||||||
|
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 Hardcoded If two Suggested approach using React's useId+import { useId } from 'react';
export const Modal: React.FC<ModalProps> = ({ isOpen, onClose, title, children, footer }) => {
const { style, mode } = useTheme();
+ const titleId = useId();
// ... variants ...
return (
<AnimatePresence>
{isOpen && (
- <div className="fixed inset-0 z-50 flex items-center justify-center p-4" role="dialog" aria-modal="true" aria-labelledby="modal-title">
+ <div className="fixed inset-0 z-50 flex items-center justify-center p-4" role="dialog" aria-modal="true" aria-labelledby={titleId}>
{/* ... */}
- <h3 id="modal-title" className={...}>{title}</h3>
+ <h3 id={titleId} className={...}>{title}</h3>🤖 Prompt for AI Agents |
||||||||||||||
| <motion.div | ||||||||||||||
| variants={overlayVariants} | ||||||||||||||
| initial="hidden" | ||||||||||||||
|
|
@@ -64,8 +64,8 @@ export const Modal: React.FC<ModalProps> = ({ isOpen, onClose, title, children, | |||||||||||||
| > | ||||||||||||||
| {/* Header */} | ||||||||||||||
| <div className={`p-6 flex justify-between items-center ${style === THEMES.NEOBRUTALISM ? 'border-b-2 border-black bg-neo-main text-white' : 'border-b border-white/10 bg-white/5'}`}> | ||||||||||||||
| <h3 className={`text-2xl font-bold ${style === THEMES.NEOBRUTALISM ? 'uppercase font-mono tracking-tighter' : ''}`}>{title}</h3> | ||||||||||||||
| <button onClick={onClose} className="hover:rotate-90 transition-transform duration-200"> | ||||||||||||||
| <h3 id="modal-title" className={`text-2xl font-bold ${style === THEMES.NEOBRUTALISM ? 'uppercase font-mono tracking-tighter' : ''}`}>{title}</h3> | ||||||||||||||
| <button onClick={onClose} className="hover:rotate-90 transition-transform duration-200" aria-label="Close modal"> | ||||||||||||||
| <X size={24} /> | ||||||||||||||
| </button> | ||||||||||||||
|
Comment on lines
+68
to
70
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. Add The close button lacks an explicit Proposed fix- <button onClick={onClose} className="hover:rotate-90 transition-transform duration-200" aria-label="Close modal">
+ <button type="button" onClick={onClose} className="hover:rotate-90 transition-transform duration-200" aria-label="Close modal">
<X size={24} />
</button>📝 Committable suggestion
Suggested change
🧰 Tools🪛 Biome (2.1.2)[error] 68-68: Provide an explicit type prop for the button element. The default type of a button is submit, which causes the submission of a form when placed inside a (lint/a11y/useButtonType) 🤖 Prompt for AI Agents |
||||||||||||||
| </div> | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import React, { createContext, useCallback, useContext, useState } from 'react'; | ||
| import { ConfirmDialog, ConfirmVariant } from '../components/ui/ConfirmDialog'; | ||
|
|
||
| interface ConfirmOptions { | ||
| title: string; | ||
| description: string; | ||
| confirmText?: string; | ||
| cancelText?: string; | ||
| variant?: ConfirmVariant; | ||
| } | ||
|
|
||
| interface ConfirmContextType { | ||
| confirm: (options: ConfirmOptions) => Promise<boolean>; | ||
| } | ||
|
|
||
| const ConfirmContext = createContext<ConfirmContextType | undefined>(undefined); | ||
|
|
||
| export const ConfirmProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| const [options, setOptions] = useState<ConfirmOptions>({ | ||
| title: '', | ||
| description: '', | ||
| }); | ||
| const [resolveRef, setResolveRef] = useState<((value: boolean) => void) | null>(null); | ||
|
|
||
| const confirm = useCallback((options: ConfirmOptions) => { | ||
| setOptions(options); | ||
| setIsOpen(true); | ||
| return new Promise<boolean>((resolve) => { | ||
| setResolveRef(() => resolve); | ||
| }); | ||
| }, []); | ||
|
Comment on lines
+26
to
+32
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 handling concurrent If Optional: Reject previous promise on new confirm call const confirm = useCallback((options: ConfirmOptions) => {
+ // Resolve any pending promise as false before opening new dialog
+ if (resolveRef) {
+ resolveRef(false);
+ setResolveRef(null);
+ }
setOptions(options);
setIsOpen(true);
return new Promise<boolean>((resolve) => {
setResolveRef(() => resolve);
});
- }, []);
+ }, [resolveRef]);🤖 Prompt for AI Agents |
||
|
|
||
| const handleConfirm = useCallback(() => { | ||
| setIsOpen(false); | ||
| if (resolveRef) { | ||
| resolveRef(true); | ||
| setResolveRef(null); | ||
| } | ||
| }, [resolveRef]); | ||
|
|
||
| const handleCancel = useCallback(() => { | ||
| setIsOpen(false); | ||
| if (resolveRef) { | ||
| resolveRef(false); | ||
| setResolveRef(null); | ||
| } | ||
| }, [resolveRef]); | ||
|
|
||
| return ( | ||
| <ConfirmContext.Provider value={{ confirm }}> | ||
| {children} | ||
| <ConfirmDialog | ||
| isOpen={isOpen} | ||
| title={options.title} | ||
| description={options.description} | ||
| confirmText={options.confirmText} | ||
| cancelText={options.cancelText} | ||
| variant={options.variant} | ||
| onConfirm={handleConfirm} | ||
| onCancel={handleCancel} | ||
| /> | ||
| </ConfirmContext.Provider> | ||
| ); | ||
| }; | ||
|
|
||
| export const useConfirm = () => { | ||
| const context = useContext(ConfirmContext); | ||
| if (context === undefined) { | ||
| throw new Error('useConfirm must be used within a ConfirmProvider'); | ||
| } | ||
| return context; | ||
| }; | ||
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.
🧹 Nitpick | 🔵 Trivial
Remove unused
modevariable.The
modevariable is destructured fromuseTheme()but is never used in this component.♻️ Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents