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

Favorites Drag and Drop Implementation #8979

Merged
merged 23 commits into from
Dec 17, 2024

Conversation

ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Dec 9, 2024

Adds drag and drop functionality for favorites management, allowing users to:

  • Move favorites between folders
  • Move favorites from folders to orphan section
  • Move orphan favorites into folders

Known Issues:
Drop detection at folder boundaries requires spacing workaround

@ehconitin ehconitin changed the title POC - Favorites drag and drop Favorites Drag and Drop Implementation Dec 13, 2024
@ehconitin ehconitin marked this pull request as ready for review December 13, 2024 11:20
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

Implemented comprehensive drag-and-drop functionality for favorites management, allowing users to reorder and move favorites between folders with visual feedback.

  • Added useReorderFavorite hook in /packages/twenty-front/src/modules/favorites/hooks/useReorderFavorite.ts to handle three distinct drag scenarios: folder header drops, inter-folder moves, and intra-folder reordering
  • Created new FavoritesDroppable component in /packages/twenty-front/src/modules/favorites/components/FavoritesDroppable.tsx with visual feedback during drag operations
  • Implemented separate droppable areas for folder headers and content in /packages/twenty-front/src/modules/favorites/components/CurrentWorkspaceMemberFavorites.tsx
  • Added temporary margin workaround in folder boundaries to fix drop detection issues
  • Integrated feature flag gating for favorites folder functionality in /packages/twenty-front/src/modules/favorites/components/CurrentWorkspaceMemberFavoritesFolders.tsx

4 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +161 to +165
marginBottom: 15,
}}
// TODO: (Drag Drop Bug) Adding bottom margin to ensure drag-to-last-position works. Need to find better solution that doesn't affect spacing.
// Issue: Without margin, dragging to last position triggers next folder drop area
>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The margin workaround could cause layout inconsistencies. Consider using a more robust solution like adjusting the drop target detection area or implementing a custom collision detection algorithm.

@charlesBochet
Copy link
Member

@ehconitin tested your PR and loved the behavior. However, there is an additional spacing below folder, is it something we want?
image

);

let newPosition;
if (destinationFavorites.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

this position things is already present in getDraggedRecordPosition, can we re-use it?

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Overall great work! I've left comments on the code

@ehconitin
Copy link
Contributor Author

@charlesBochet Tests related to this PR are done!
I am still not sure how we get around this issue https://github.com/twentyhq/twenty/pull/8979/files#diff-43b355cf0cd133b768296643d0b7499f08dde8de0adcce1bf704ac7129742337R164-R165

This is regarding the extra padding below each folder. Do you have any ideas?

@charlesBochet charlesBochet merged commit 582530e into twentyhq:main Dec 17, 2024
20 checks passed
Copy link

Thanks @ehconitin for your contribution!
This marks your 72nd PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

Copy link

sentry-io bot commented Dec 20, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: Cannot read properties of undefined (reading 'position') /settings/objects/listings View Issue

Did you find this useful? React with a 👍 or 👎

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.

3 participants