-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: add delete confirmation dialog before real delete to prevent unexpected operation #1197
feat: add delete confirmation dialog before real delete to prevent unexpected operation #1197
Conversation
…expected operation
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.
Thank you very much for your contribution!
The PR looks good, however, if possible, could you also replace the usage of the confirmDelete
modal in config and chatMessages with the same approach?
Before this can be merged, you'll also need to run npm run-script build
I just checked the config and chatMessage, there are existing modal with some special feature: do you think we should keep them? or need make the new shared service support these special feature? |
@xinatcg it would be ideal if we can retain the current behavior, since these are specialized I don't think we need a shared service. |
Ok, if so
Should we leave them out and only keep the code for others without confirmation? |
I'll approve without those changes, but it would be good to touch on them as part of this PR, if possible. I think those modals aren't being displayed correctly due to the upgrade to Angular 15 changes. |
make some updates to support formGroup with 2 control in the confirm component |
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.
Left a couple of suggestions but otherwise LGTM 👍
Co-authored-by: Simon Esposito <[email protected]>
Co-authored-by: Simon Esposito <[email protected]>
update with 2 suggestions. thanks |
Please rerun |
done |
currently, all modal messages are the same, but the service supports custom titles and content for different usage.
#1123