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

Add Confirmation Modal for Deletion Action and Map All Action Bar Entries #6357

Conversation

falko100
Copy link
Contributor

This pull request addresses an issue where the delete button, placed closely to the export button, could lead to accidental deletions without confirmation. The problem was that the confirmation modal for the delete action, being the second action, was not displayed. The code was updated to map all action bar entries and display their respective confirmation modals correctly.

Changes Made:

  • Updated the rendering of action bar entries to ensure all confirmation modals are shown.
    Changed from displaying only the first action bar entry's confirmation modal to mapping through all entries and displaying each modal if it exists.
  • Removed the delete from the onClick because it is present in the onConfirmClick.

…ove handleDeleteClick from useRecordActionBar.
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Ensured all action bar entries display their respective confirmation modals, particularly addressing the delete action.

  • Modified packages/twenty-front/src/modules/object-record/record-action-bar/hooks/useRecordActionBar.tsx to map through all action bar entries and display each confirmation modal if it exists.
  • Removed the delete action from the onClick handler and moved it to the onConfirmClick handler.
  • Updated packages/twenty-front/src/modules/ui/navigation/shared/components/NavigationModal.tsx to render confirmation modals for all action bar entries.

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@ijreilly
Copy link
Collaborator

@falko100 thanks for your contribution! Just checking-in with product if they are fine with this behavior and warning message.

@Bonapara what do you think? Is this something we will want for all deletion operations (eg field/object deletion in the data model)?

Enregistrement.de.l.ecran.2024-07-22.a.12.17.40.mov

@Bonapara
Copy link
Member

@ijreilly I think this is okay for now, as there is a kind of soft-delete (Disabling) for the fields and object deletion. However, for records, there is no soft delete yet.

@ijreilly ijreilly merged commit f24536c into twentyhq:main Jul 22, 2024
3 checks passed
@onavhamid
Copy link

onavhamid commented Aug 1, 2024

Hi. Did this PR add a warning when deleting an ppportunity? (i.e. using the upper-right button)
image

Recently tried to delete a Task inside an opportunity and accidentally deleted the entire opportunity. Having a confirmation or warning would have helped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants