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

Row dragging capabilities #616

Merged
merged 26 commits into from
Jul 30, 2024
Merged

Row dragging capabilities #616

merged 26 commits into from
Jul 30, 2024

Conversation

MohdAhmad1
Copy link
Contributor

@MohdAhmad1 MohdAhmad1 commented Jul 4, 2024

This PR introduces row dragging capabilities to the Mantine Datatable component. This feature was developed to meet a requirement in our organization and can benefit the wider Mantine community by providing an intuitive way to reorder rows within the datatable.

closes #513

Key Features:

  • Row Dragging: Users can now drag and drop rows to reorder them or swap them within the datatable.
  • Customizable Drag Handles: Optional drag handles can be added for better user control.
  • Event Callbacks: New event callback for onDragEnd to handle custom logic after the drag-and-drop process.
  • Enhanced User Experience: Improved visual feedback during dragging for a more intuitive interaction.

Implementation Details:

  • Utilized the @hello-pangea/dnd to handle drag-and-drop functionality.
  • Ensured compatibility with existing datatable features and customization options.
  • Added comprehensive documentation and examples to demonstrate the new feature.

Documentation:

  • Updated the Mantine Datatable documentation to include usage instructions and examples for the row dragging feature.

We believe this enhancement will significantly improve the flexibility and usability of the Mantine Datatable component. We welcome any feedback and suggestions for further improvements. Thank you for considering this PR.

Copy link

codesandbox bot commented Jul 4, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@MrSharpp
Copy link

MrSharpp commented Jul 7, 2024

i need this feature, can anyone please merge?

@MohdAhmad1 MohdAhmad1 changed the base branch from main to next July 8, 2024 13:25
@icflorescu
Copy link
Owner

This definitely implements a really useful piece of functionality that has been requested by user before.

The only thing that really bothers me is that we'd need to add @hello-pangea/dnd as a mandatory dependency, regardless of whether developers are using the DnD feature or not... which also contradicts the current package "dependency-free" philosophy.

I'm thinking perhaps we should reserve this for a new major release... Otherwise we'd make lot of applications break if users will update their deps without realizing that we have introduced a new dependency.

Or maybe we could find a way to make the dependency optional, so that only users who need DnD are forced to install it?...

I'm really curious to hear other people's feedback on this before making a decision.

@icflorescu icflorescu added enhancement New feature or request help wanted Extra attention is needed possible discussion Not exactly a bug or a feature request, but rather something that could or should be discussed labels Jul 10, 2024
@mikkurogue
Copy link

Wouldn't it be possible to make the drag and drop functionality an extra module that a user can install alongside the mantine-datatable package? Essentially making it an opt-in package that then would use the @hello-pangea/dnd dependency but then the user is explicitly installing this for the reason of using the functionality.

Another idea I have, but would have to be tested pretty well, is to install the @hello-pangea/dnd (or any other dnd package) using the --save-optional flag. I'm not totally sure if this would result in a direct always installed dependency or only if the feature-set being used requires the dependency.

I'll take some time to research optional dependencies and modules for packages.

@mikkurogue
Copy link

Also, I would like to point out that installing @pangea-hello/dnd would result in pulling in 18 extra dependencies, as can be seen here: https://npmgraph.js.org/?q=%40hello-pangea%2Fdnd

In case you value keeping dependency count manageable

change link @hello-pangea/dnd on example page
change structure this row dragging feature
Add row factory feature to Data Table
@mehdiraized
Copy link
Contributor

i fixed dependency on this commit
ee6b61b

@MohdAhmad1
Copy link
Contributor Author

MohdAhmad1 commented Jul 16, 2024

i fixed dependency on this commit ee6b61b

I like the idea of rowFactory, I have a suggestion to improve this

  • we can take children (array of cells) and tr props (provided by mantine datatable) in row factory function arguments to keep it DRY

in this way its usage will be something similar to

<DataTable
  //other props
  rowFactory={({ record, index, trProps, children }) => (
    <Draggable key={record.id} draggableId={record.id} index={index}>
      {(provided) => (
        <Table.Tr
          ref={provided.innerRef}
          {...provided.draggableProps}
          {...provided.dragHandleProps}
          {...trProps}
        >
          {/** add a td for drag handle (optional) */}
          {children}
        </Table.Tr>
      )}
    </Draggable>
  )}
/>;

any comments?

@mehdiraized
Copy link
Contributor

i fixed dependency on this commit ee6b61b

I like the idea of rowFactory, I have a suggestion to improve this

  • we can take children (array of cells) and tr props (provided by mantine datatable) in row factory function arguments to keep it DRY

in this way its usage will be something similar to

<DataTable
  //other props
  rowFactory={({ record, index, trProps, children }) => (
    <Draggable key={record.id} draggableId={record.id} index={index}>
      {(provided) => (
        <Table.Tr
          ref={provided.innerRef}
          {...provided.draggableProps}
          {...provided.dragHandleProps}
          {...trProps}
        >
          {/** add a td for drag handle (optional) */}
          {children}
        </Table.Tr>
      )}
    </Draggable>
  )}
/>;

any comments?

yes its ok
can you add rowFactory to document and merge PR and make new version for test production ?

so  in previous solution if user scrolls in table while dragging, it disrupts the calculation of dnd library, to tackle that issue I've added tableWrapper prop to add a wrapper inside scrollarea
@MohdAhmad1
Copy link
Contributor Author

MohdAhmad1 commented Jul 17, 2024

@mehdiraized I did some change in your code, and update example as well, any comments on it?

@mehdiraized
Copy link
Contributor

@mehdiraized I did some change in your code, and update example as well, any comments on it?

no i haven't any comment
i close my PR and this PR is good update
@icflorescu can you check this PR ?

@josephycyeh
Copy link

josephycyeh commented Jul 29, 2024

@icflorescu can we add this feature?

@icflorescu
Copy link
Owner

Looks fantastic, thanks, @MohdAhmad1!
Will publish a release today.

style={{
cursor: 'grab',

width: '50px',
Copy link
Owner

Choose a reason for hiding this comment

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

To be honest, I'm not 100% happy with the default 50px width. Ideally the drag handle column should take as little space as possible.

@icflorescu icflorescu merged commit b30f06a into icflorescu:next Jul 30, 2024
@sidpagariya
Copy link

Not sure if #625 is a direct result of this merge but wanted to see your thoughts on that @icflorescu since the behavior wasn't broken before

@Nishchit14
Copy link

@hello-pangea/[email protected] will increase the size in double for this finest library. and It'll unnecessary add redux in the code base. All this cost is just for the row reorder.

Isn't there any other solution to implement the row reorder?

image image

@mehdiraized
Copy link
Contributor

@hello-pangea/[email protected] will increase the size in double for this finest library. and It'll unnecessary add redux in the code base. All this cost is just for the row reorder.

Isn't there any other solution to implement the row reorder?

image image

we just added rowFactory props
you can use other packages for this feature

@mikkurogue
Copy link

we just added rowFactory props you can use other packages for this feature

I think the thing here is that even if you only use 1 feature of the package, it still pulls in all peer dependencies.

I think row re-ordering is easily achieved without having to implement an entire library. Especially when people like me would not use the row re-ordering functionality and yet I now have to pull in the extra dependency if I were to udate to the latest version

@mikkurogue
Copy link

And to just mention this, I did say in a previous comment that this will pull in a lot of extra deps: #616 (comment)

@Nishchit14
Copy link

Nishchit14 commented Aug 6, 2024

I agree with you @mikkurogue

@mehdiraized you're right but if we plan to make this reorder feature the lib agnostic then we should at least build this feature with two different dnd libraries to prove that it works perfectly without any unforeseen issue.

@mehdiraized
Copy link
Contributor

i can't understanding what is problem
this package is large ok use other packages in your project
mantine-datatable is not peerdependency of @hello-pangea/dnd package
you can replace @hello-pangea/dnd package with other drag and drop package
we make example row drag and drop with this package becuase this package is popular
but you can use rowFactory props and use other light package

@mehdiraized
Copy link
Contributor

you say if you update to last version mantine-datatable we automatically added @hello-pangea/dnd package to your project
i think its not true
because this package is not peerDependency like @formkit/auto-animate

@mikkurogue
Copy link

you say if you update to last version mantine-datatable we automatically added @hello-pangea/dnd package to your project
i think its not true

yeah after checking you are right, it should not include the @hello-pangea/dnd package by itself.

I do still think a native solution is still probably better, as this feature then doesn't introduce dnd functionality, just introduces the ability to implement it yourself (so to speak)

@mehdiraized
Copy link
Contributor

you say if you update to last version mantine-datatable we automatically added @hello-pangea/dnd package to your project
i think its not true

yeah after checking you are right, it should not include the @hello-pangea/dnd package by itself.

I do still think a native solution is still probably better, as this feature then doesn't introduce dnd functionality, just introduces the ability to implement it yourself (so to speak)

if we can add this feature by other packages
Absolutely you can added drag and drop feature by native js code
but is not relate with this PR this is new feature request
i think you can get native drag and drop feature with gpt or claude with rowFactory feature they can create native js code for drag and drop

@mikkurogue
Copy link

Agreed, but then I think its a different discussion to use gpt/claude/copilot to generate the implementation (Im a certified copilot refuser so I'm biased).

I think, personally the value of a feature request shouldn't lie in an abstract "we allow you to implement it yourself" when the functionality people want is usually the same.

End of the day its icflorescu's call, but I still do doubt the value of this introduction when it just exposes an API to implement it yourself (unless I'm misunderstanding this)

@mehdiraized
Copy link
Contributor

Agreed, but then I think its a different discussion to use gpt/claude/copilot to generate the implementation (Im a certified copilot refuser so I'm biased).

I think, personally the value of a feature request shouldn't lie in an abstract "we allow you to implement it yourself" when the functionality people want is usually the same.

End of the day its icflorescu's call, but I still do doubt the value of this introduction when it just exposes an API to implement it yourself (unless I'm misunderstanding this)

This feature is not about me
I just contributed with this PR
We just took one step for this feature
And I think this is the right step
Because the complex features should work very simply

@mikkurogue
Copy link

Agreed, but then I think its a different discussion to use gpt/claude/copilot to generate the implementation (Im a certified copilot refuser so I'm biased).
I think, personally the value of a feature request shouldn't lie in an abstract "we allow you to implement it yourself" when the functionality people want is usually the same.
End of the day its icflorescu's call, but I still do doubt the value of this introduction when it just exposes an API to implement it yourself (unless I'm misunderstanding this)

This feature is not about me I just contributed with this PR We just took one step for this feature And I think this is the right step Because the complex features should work very simply

I think you're misunderstanding what I'm saying.

The contribution is fine, but I feel its disingenous to call it a feature when it doesnt introduce any new functionality outside of allowing a user to hook their own library into the factory. As much as it is cool, it certainly doesn't feel like it's the way forward for the library to introduce new features if it still boils down to me having to still implement it myself at the end of the day.

I agree complex features should work very simply, but the question then is; what do you classify as a complex feature? Any drag and drop I don't consider complex (a pain sometimes).

Personally I don't think a library that delivers functionality shouldn't expose an API to the user to introduce external functionality, at that point it's just forking the repo with extra steps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed possible discussion Not exactly a bug or a feature request, but rather something that could or should be discussed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

drag and drop with @hello-pangea/dnd
8 participants