Skip to content
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

EES-5568 Adding modal for admins deleting users #5342

Merged
merged 3 commits into from
Oct 23, 2024
Merged

EES-5568 Adding modal for admins deleting users #5342

merged 3 commits into from
Oct 23, 2024

Conversation

jack-hive
Copy link
Collaborator

@jack-hive jack-hive commented Oct 16, 2024

When BAU users try to delete users from Admin, we need to warn users to confirm this as it's a fairly big action.
This PR adds a confirmation modal to confirm the deletion before they proceed.

Notes for Reviewer

  • I'm not sure if I've put the modal component in the right place. Nor am I sure if there was another component I should have re-used instead. Please let me know if there was something different I should have done here.
  • The code is commented out as we still want to hide this Delete button for now until the issue in EES-5573 is resolved.
  • Please uncomment my code, add the necessary imports, and re-test it for me. I'm new to React so I may have missed something.
  • Thank you

Comment on lines 9 to 11
const DeleteUserModal = ({ triggerButton, onConfirm }: Props) => {
return (
<ModalConfirm
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! This is absolutely fine to do, and works perfectly well. I just wanted to add my personal opinion to give some food for thought on your frontend journey!

We didn't have a DeleteUserButton previously, I think for the same reasons I would not create a DeleteUserModal. As the DeleteUserModal doesn't do much other than simply return another component, I think I would avoid creating a new component and use the ModalConfirm in BauUsersPage directly. Doing so will have less lines added and less files added. However, if this modal is to appear across the service, then I'd opt for the component separation for easier tweaking later on. Hope this helps!

Copy link
Collaborator Author

@jack-hive jack-hive Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Stuart!

I completely agree, that sounds like a more sensible approach to me! I think I did it like this because I saw that we split out a custom component for the same thing elsewhere, which was also only used by one other component. I was torn at the time purely for that reason, but your comment has now re-confirmed in my mind that it's better to not split out a custom component for this, as it is only used in one place :)

Copy link
Collaborator

@bennettstuart bennettstuart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, left an opinion comment to think about but implementation is all gooood!

@jack-hive jack-hive merged commit cf62a0d into dev Oct 23, 2024
7 checks passed
@jack-hive jack-hive deleted the EES-5568 branch October 23, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants