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

Fix: Prevent element dropping below 'New' Button #10412

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

Conversation

SujithThirumalaisamy
Copy link
Contributor

#10197
Issue: Elements were dropped after the "New" button, And placed back to last index after api refresh.
Solution: Moved the "New" button out of the Draggable to prevent the issue.

Previous behaviour: Reference #10197
Current behavious: Screencast.webm

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

This PR fixes an issue in the Kanban board where draggable elements could be incorrectly dropped below the "New" button, causing inconsistent behavior after API refresh.

  • Moved StyledNewButtonContainer outside of the draggable area in /packages/twenty-front/src/modules/object-record/record-board/record-board-column/components/RecordBoardColumnCardsContainer.tsx to prevent cards from being dropped after the "New" button
  • Added isDragDisabled={true} to the bottom Draggable wrapper to ensure consistent behavior
  • Ensures cards maintain their intended order within the droppable area by keeping the "New" button as a static element at the bottom

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

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

@prastoin prastoin self-requested a review February 26, 2025 09:33
Copy link
Contributor

@prastoin prastoin left a comment

Choose a reason for hiding this comment

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

Introduction

Hey @SujithThirumalaisamy, thanks for your contribution !

Pros

In my opinion it does not really solve the issue but improves current behavior as now allows a smooth noop animation instead of weird flickering

Screen.Recording.2025-02-26.at.11.07.23.mov

Cons

It seems to be introducing very low tolerance regarding whereas the current card is within the draggable container or not, fallbacking as previously when under the add new CTA 🤔 We could a bottom minimal padding to the draggable container to counter this.

Screen.Recording.2025-02-26.at.10.58.29.mov

Conclusion

I would approve but not close the issue for the moment, after resolving the new small issue. Unless @Bonapara considers this is enough and going further might be overkill ?
Please let me know

@SujithThirumalaisamy
Copy link
Contributor Author

I can try the suggestion that you have given. Will update it in few hours

@Bonapara
Copy link
Member

We should mimic this behavior (with smoother transitions)
https://github.com/user-attachments/assets/5477ccbe-c5dc-46e7-9194-b2eaca7efb69

@prastoin
Copy link
Contributor

prastoin commented Feb 26, 2025

I can try the suggestion that you have given. Will update it in few hours

Lets land on a behavior before starting a new implementation
From what @Bonapara said I understand than moving the new CTA might not or is not enough to fulfill our need here.
I don't know what would be the most idiomatic to hello-pangea 🤔 We should have a deeper look into airtable integration in order to determine what should be in the container or not

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.

3 participants