🎨 Palette: Add confirmation for deleting sections#493
Conversation
Added ResponsiveConfirmDialog to SectionControls.tsx to prevent accidental deletion of resume sections. Co-authored-by: aafre <8656674+aafre@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a confirmation dialog for deleting sections within the SectionControls component. The review feedback points out a redundant state update in the deletion handler and identifies a missing focus trap in the ResponsiveConfirmDialog component, which is an important accessibility requirement.
| setSections(newSections); | ||
| setIsConfirmOpen(false); |
There was a problem hiding this comment.
The call to setIsConfirmOpen(false) is redundant here because the ResponsiveConfirmDialog component already calls its onClose prop (which updates this state) immediately after the onConfirm callback finishes. Additionally, since the section is being deleted, this component will likely be unmounted, making the state update unnecessary.
| setSections(newSections); | |
| setIsConfirmOpen(false); | |
| setSections(newSections); |
| <ResponsiveConfirmDialog | ||
| isOpen={isConfirmOpen} | ||
| onClose={() => setIsConfirmOpen(false)} | ||
| onConfirm={deleteSection} | ||
| title="Delete Section" | ||
| message="Are you sure you want to delete this section? This action cannot be undone." | ||
| confirmText="Delete Section" | ||
| cancelText="Cancel" | ||
| isDestructive={true} | ||
| /> |
There was a problem hiding this comment.
The ResponsiveConfirmDialog component does not implement a focus trap, which is required by the accessibility guidelines to ensure keyboard focus remains within the modal while it is open. Please update the ResponsiveConfirmDialog component to include a focus trap (e.g., using a library like react-focus-lock or manual focus management).
References
- For accessibility, all modals must implement a focus trap to ensure keyboard focus remains within the modal's focusable elements while it is open.
|
Closing duplicate — the most recent version of this change is PR #543. |
1 participant
💡 What: Added a
ResponsiveConfirmDialogconfirmation prompt before deleting a resume section.🎯 Why: To prevent accidental data loss when users click the delete section button.
📸 Before/After: Before, clicking delete immediately removed the section. After, a standard, mobile-friendly confirmation dialog requires user confirmation.
♿ Accessibility: Leverages the existing
ResponsiveConfirmDialogwhich handles properrole="dialog", ARIA attributes, and focus management.PR created automatically by Jules for task 18413180632263056216 started by @aafre