-
Notifications
You must be signed in to change notification settings - Fork 544
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
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (7)
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: 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.
"this_will_permanently_remove_the_template", | ||
{ | ||
data: "session", | ||
}, |
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.
why make this variables?
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.
@rithviknishad @Jacobjeevan "this_will_permanently_remove_the_template": "This will permanently remove the {{data}} and cannot be undone",
i tried to add dynamic way,
should i make different translation text for each pop-up (eg: this_will_permanently_remove_the_template
)
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Proposed Changes
Vinu.TV._.CARE.-.confirm.dialog.2025-01-31.01-44-05.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit