✨ Questionnaire delete confirmation

[avivtur]

closes #1276

Adding an explanation of what happens if the questionnaire is deleted and forcing the user to write the questionnaire's name to deletion.

Note: the input is case-sensitive to the name of the questionnaire.

Before:

After:

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (d4b606b) 42.58% compared to head (3c03c43) 42.59%.

❗ Current head 3c03c43 differs from pull request most recent head c61ddce. Consider uploading reports for the commit c61ddce to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1303      +/-   ##
==========================================
+ Coverage   42.58%   42.59%   +0.01%     
==========================================
  Files         136      136              
  Lines        4220     4221       +1     
  Branches     1000     1001       +1     
==========================================
+ Hits         1797     1798       +1     
  Misses       2411     2411              
  Partials       12       12              

Flag	Coverage Δ
client	42.59% <100.00%> (+0.01%)	⬆️
server	∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
client/src/app/components/ConfirmDialog.tsx	90.90% <100.00%> (+0.90%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sjd78]

Issues I found:

• The Cancel button on the delete confirm modal is never enabled, but the X icon and the Esc key do work.

• In other "type the name of the thing to delete" interactions I've used, the name of the thing is provided on the modal. Right now, if I don't remember the exact full name of the assessment questionnaire, I need to close the confirm box copy the name, then go back in, then paste it. Adding the name questionnaire being deleted to the confirm modal would help.

[avivtur]

Hi @sjd78
Thank you for taking the time to review this PR! 😄

Issues I found:

• The Cancel button on the delete confirm modal is never enabled, but the X icon and the Esc key do work.

That's not entirely accurate, the Cancel button is enabled only when submit is enabled as well, but I agree that that is not a wanted behavior, I should expose a new prop the ConfirmModal component to disable the submit only.

• In other "type the name of the thing to delete" interactions I've used, the name of the thing is provided on the modal. Right now, if I don't remember the exact full name of the assessment questionnaire, I need to close the confirm box copy the name, then go back in, then paste it. Adding the name questionnaire being deleted to the confirm modal would help.

Agreed, will follow the example from Console "Delete project" modal:

[sjd78]

The confirm deletion with the name of the entity to delete right there is perfect.

But looking more closely at the ConfirmDialog component itself, I think this use is beyond what ConfirmDialog is really meant for. WDYT about just using a Modal or enhancing DeleteQuestionnaireMessage to ConfirmDeleteQuestionnaireModal? That way the cancel button and the submit button can work exactly as we want without impacting any other use of ConfirmDialog that really is just a message with Yes/No button kind of thing.

With ConfirmDeleteQuestionnaireModal, it wouldn't need to be a controlled component. All the state logic could just live in the modal.

Maybe it seems like splitting hairs a bit, but I think it'll look nicer that way.

WDYT @ibolton336, @gildub, @mturley ?

[mturley]

@sjd78 I agree, I think there's a cleaner abstraction to be found here. I wonder if we are going to ever want another entity with type-the-name-to-confirm-deletion in this way? If so, maybe we should make a ConfirmDeleteModal or ConfirmDeleteWithNameModal that either simply wraps ConfirmDialog with the state/logic that is currently in the page here, or uses its own Modal instead of ConfirmDialog. Then we can have a ConfirmDeleteQuestionnaireModal that is a trivial usage of it, or just inline that usage in the page (since it would encapsulate all the state).

[avivtur]

@mturley @sjd78 @ibolton336 Hi there
Can you please review it again?

[sjd78]

Functionally looks much better. Just a few extra minor tweaks needed.

[sjd78]

You can drop the changes on this file now.

[avivtur]

Right, will revert changes

[sjd78]

Either this key needs to be fixed to something like `"dialog.aria.confirmDelete" or just use the English string. I've not seen any other externalized aria labels.

[avivtur]

Right, habit from CNV, will change

[sjd78]

LGTM