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

Delete button in right drawer / side pannel #7200

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ehconitin
Copy link
Contributor

fixes #7069
@Bonapara

2024-09-23.17-42-55.mp4

Copy link

@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

This PR adds a delete button to the right drawer/side panel of the show page, addressing issue #7069 by allowing users to delete notes/tasks directly from the side panel.

  • Implemented delete functionality in packages/twenty-front/src/modules/ui/layout/show-page/components/ShowPageRightContainer.tsx
  • Added a styled component StyledButtonContainer for consistent button placement
  • Utilized useDeleteOneRecord hook for record deletion
  • Conditionally rendered the delete button when the component is in the right drawer
  • Improved user experience by enabling quick deletion without navigating to the Notes object list

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

@bosiraphael bosiraphael self-assigned this Sep 23, 2024
@bosiraphael
Copy link
Contributor

Hello @ehconitin ! It seems that the trash Icon is missing in the button ;)

@ehconitin
Copy link
Contributor Author

@bosiraphael oops, I am yet to finalize changes Raphael.
In #7168, I think I forgot to remove code which was to be addressed in that PR itself. I dont know how I missed it.
That issue needs to be addressed!
So right now I am thinking of a way I could handle that in this PR as these both buttons have similiar styles.
Ill tag you once done

Copy link
Contributor

@bosiraphael bosiraphael left a comment

Choose a reason for hiding this comment

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

Left some comments :)

@bosiraphael
Copy link
Contributor

bosiraphael commented Sep 23, 2024

@bosiraphael oops, I am yet to finalize changes Raphael. In #7168, I think I forgot to remove code which was to be addressed in that PR itself. I dont know how I missed it. That issue needs to be addressed! So right now I am thinking of a way I could handle that in this PR as these both buttons have similiar styles. Ill tag you once done

Okay no worries! When you are still working on your PR, you can open a draft PR instead, or just add the label WIP :)

@ehconitin
Copy link
Contributor Author

@bosiraphael oops, I am yet to finalize changes Raphael. In #7168, I think I forgot to remove code which was to be addressed in that PR itself. I dont know how I missed it. That issue needs to be addressed! So right now I am thinking of a way I could handle that in this PR as these both buttons have similiar styles. Ill tag you once done

Okay no worries! When you are still working on your PR, you can open a draft PR instead, or just add the label WIP :)

@bosiraphael yup yup, will take a note of it for next time :)

@ehconitin
Copy link
Contributor Author

@bosiraphael
for command menu to be alligned for mobile devices we do this, so should I follow the same pattern or having navigationBar height state makes more sense, so that we dont have to hardcode this value each time?

@bosiraphael
Copy link
Contributor

@bosiraphael for command menu to be alligned for mobile devices we do this, so should I follow the same pattern or having navigationBar height state makes more sense, so that we dont have to hardcode this value each time?

I think the issue of the button being missaligned has something to do with the ShowPageRightContainer not being the right size, I don't think a state is the correct pattern but you can use a constant if you find it useful :)

@ehconitin
Copy link
Contributor Author

ehconitin commented Sep 25, 2024

@FelixMalfait
I narrowed down the issue, useDeleteOneRecord was not refetching queries like useRestoreManyRecords does.
I followed the same pattern from useRestoreManyRecords and it worked but within useRestoreManyRecords there is a comment above those refetch queries

 // TODO: fix optimistic effect
      const findOneQueryName = `FindOne${capitalize(objectNameSingular)}`;
      const findManyQueryName = `FindMany${capitalize(
        objectMetadataItem.namePlural,
      )}`;
2024-09-25.18-10-15.mp4


const StyledRightDrawerRecord = styled.div`
height: ${({ theme }) =>
useIsMobile() ? `calc(100% - ${theme.spacing(16)})` : '100%'};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bosiraphael
This is correct right? this is regarding the height of the right drawer.
Should i just use a local const like in CommandMenu? or create new const file and export it from there to both the files

Copy link
Contributor

Choose a reason for hiding this comment

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

Second option is better to avoid code duplication :)

@FelixMalfait
Copy link
Member

@ehconitin yes re-fetching is not a best practise, it throws an additional request to fetch the resource again, while we should instead be able to optimistically predict the result and modify the cache based on that

@ehconitin
Copy link
Contributor Author

Hi @FelixMalfait, I don't have much expertise in this area, unfortunately.

@bosiraphael, could you assist with this? For more context, please follow the comment thread here.

@FelixMalfait
Copy link
Member

As discussed in Discord, since we introduce Soft Delete, the delete isn't really a delete anymore, so we should instead leverage what's been done for optimistic updates when there's a delete, as what we call "delete" is actually closer to an update.
We should keep the code for optimistic delete but instead rename it and use it in the case of "destroy" (our internal name for "hard deletes")

@ehconitin
Copy link
Contributor Author

@FelixMalfait Understood, I am still thinking about it :(. Ill get back to you!

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

Successfully merging this pull request may close these issues.

Delete a note/task from side panel
3 participants