-
Notifications
You must be signed in to change notification settings - Fork 543
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
Add Confirmation Pop-up to Prevent Accidental Exception Deletions #10283
base: develop
Are you sure you want to change the base?
Add Confirmation Pop-up to Prevent Accidental Exception Deletions #10283
Conversation
WalkthroughThe changes introduce confirmation dialogs in various scheduling and appointment interfaces to require explicit user confirmation before deletion actions. Modifications include new state variables, updated mutation calls, and enhanced localization messages. These changes span schedule exceptions, schedule templates, and availability entries, ensuring that users are prompted before any irreversible deletion is performed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Backend
User->>UI: Click delete action (exception/template/availability)
UI->>UI: Open confirmation dialog
alt User confirms deletion
UI->>Backend: Execute deletion mutation
Backend-->>UI: Return success
UI->>UI: Close dialog
else User cancels deletion
UI->>UI: Simply close dialog
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.
|
@modamaan what is the status on the PR |
@rithviknishad #10048 Has work started on the ConfirmDialog? |
@Jacobjeevan, can we use the existing |
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/Scheduling/components/EditScheduleTemplateSheet.tsx (1)
284-286
: Make the warning message more specific.The current warning message is generic. Consider making it more specific to help users understand exactly what they're deleting.
- {t("this_will_permanently_remove_the_template")} + {t("this_will_permanently_remove_template", { name: template.name })}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx
(4 hunks)src/pages/Scheduling/components/EditScheduleTemplateSheet.tsx
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
src/pages/Scheduling/components/EditScheduleTemplateSheet.tsx (2)
18-29
: LGTM! Clean import organization.The new Alert and AlertDialog component imports are well-organized and correctly sourced from the UI library.
141-141
: LGTM! Well-implemented confirmation dialog.The AlertDialog implementation for template deletion is clean and follows best practices with proper state management.
Also applies to: 265-303
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Appointments/AppointmentDetail.tsx (1)
532-561
: LGTM! Good UX improvements for destructive actions.The addition of
variant="destructive"
to the confirmation buttons enhances the visual feedback, making it clearer that these are destructive actions. This aligns well with the PR's goal of preventing accidental deletions.Consider these additional UX improvements:
- Add a brief delay (e.g., 2 seconds) before enabling the confirm button to prevent accidental clicks.
- Require typing a confirmation word (e.g., "DELETE") for extra validation.
Example implementation for the delay:
<AlertDialogAction variant="destructive" + disabled={!useEffect(() => { + const timer = setTimeout(() => setEnabled(true), 2000); + return () => clearTimeout(timer); + }, [])} onClick={() => cancelAppointment({ reason: "cancelled" })} > {t("confirm")} </AlertDialogAction>Also applies to: 563-592
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json
(2 hunks)src/pages/Appointments/AppointmentDetail.tsx
(2 hunks)src/pages/Scheduling/ScheduleExceptions.tsx
(4 hunks)src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx
(4 hunks)src/pages/Scheduling/components/EditScheduleTemplateSheet.tsx
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx
- src/pages/Scheduling/ScheduleExceptions.tsx
- public/locale/en.json
- src/pages/Scheduling/components/EditScheduleTemplateSheet.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
…modamaan/care_feAmaan into issues/10259/confirmation-popup
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Scheduling/components/EditScheduleTemplateSheet.tsx (2)
290-292
: Fix typo in template name.The warning message contains a typo in the template name: "scheduled teplate".
- data: "scheduled teplate", + data: "schedule template",
410-418
: Remove redundant onClick handler.The
onClick
handler is redundant asAlertDialogTrigger
already manages the dialog state.<Button variant="ghost" size="icon" - onClick={() => setOpenDialog(true)} disabled={isDeleting} > <CareIcon icon="l-trash" className="text-lg" /> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/pages/Appointments/AppointmentDetail.tsx
(3 hunks)src/pages/Scheduling/ScheduleExceptions.tsx
(4 hunks)src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx
(3 hunks)src/pages/Scheduling/components/EditScheduleTemplateSheet.tsx
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/Scheduling/ScheduleExceptions.tsx
- src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx
- src/pages/Appointments/AppointmentDetail.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Scheduling/components/EditScheduleTemplateSheet.tsx (3)
12-32
: LGTM! Clean import organization.The new imports for Alert and AlertDialog components are well-organized and necessary for implementing the confirmation dialogs.
267-312
: LGTM! Well-implemented confirmation dialogs.The implementation of confirmation dialogs for both template and availability deletions successfully addresses the PR objective of preventing accidental deletions. The dialogs provide clear warnings and require explicit user confirmation before proceeding with deletion actions.
Also applies to: 405-448
426-428
:⚠️ Potential issueFix incorrect warning message.
The warning message incorrectly refers to template deletion instead of availability deletion.
- {t("this_will_permanently_remove_the_template", { - data: "available session", + {t("this_will_permanently_remove_availability", { + name: availability.name,Likely invalid or redundant comment.
Proposed Changes
Vinu.TV._.CARE.-.confirm.dialog.2025-01-31.01-44-05.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit