search

heroiclabs / nakama

Distributed server for social and realtime games and apps.
https://heroiclabs.com
Apache License 2.0
8.58k stars 1.06k forks source link

feat: add delete confirmation dialog before real delete to prevent unexpected operation #1197

Closed xinatcg closed 2 months ago

xinatcg commented 2 months ago
image

currently, all modal messages are the same, but the service supports custom titles and content for different usage.

https://github.com/heroiclabs/nakama/issues/1123

xinatcg commented 2 months ago

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: config: need Type 'DELETE' to confirm https://github.com/xinatcg/nakama/blob/40b94f02ba508d9f816250f697525fdc741ec5f4/console/ui/src/app/config/config.component.html#L103-L110 chatMessage: Choose how many days to retain: https://github.com/xinatcg/nakama/blob/40b94f02ba508d9f816250f697525fdc741ec5f4/console/ui/src/app/channels/chatMessages.component.html#L142-L151

do you think we should keep them? or need make the new shared service support these special feature?

sesposito commented 2 months ago

@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.

xinatcg commented 2 months ago

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?

Ok, if so

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?

Should we leave them out and only keep the code for others without confirmation?

sesposito commented 2 months ago

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.

xinatcg commented 2 months ago

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

xinatcg commented 2 months ago

Left a couple of suggestions but otherwise LGTM 👍

update with 2 suggestions. thanks

sesposito commented 2 months ago

Please rerun npm run build and we'll get this merged 👍

xinatcg commented 2 months ago

Please rerun npm run build and we'll get this merged 👍

done