Row dragging capabilities

MohdAhmad1 commented 2 months ago

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.

codesandbox[bot] commented 2 months ago

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders
Open Preview

MrSharpp commented 2 months ago

i need this feature, can anyone please merge?

icflorescu commented 2 months ago

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.

mikkurogue commented 2 months ago

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 commented 2 months ago

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

mehdiraized commented 2 months ago

i fixed dependency on this commit https://github.com/icflorescu/mantine-datatable/pull/621/commits/ee6b61bfb3ac8221b190078ebf4eee808e48312f

MohdAhmad1 commented 2 months ago

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 commented 2 months ago

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 ?

MohdAhmad1 commented 2 months ago

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

mehdiraized commented 2 months ago

@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 commented 2 months ago

@icflorescu can we add this feature?

icflorescu commented 2 months ago

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

sidpagariya commented 2 months ago

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 commented 1 month ago

@hello-pangea/dnd@16.6.0 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?

mehdiraized commented 1 month ago

@hello-pangea/dnd@16.6.0 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?

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

mikkurogue commented 1 month ago

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 commented 1 month ago

And to just mention this, I did say in a previous comment that this will pull in a lot of extra deps: https://github.com/icflorescu/mantine-datatable/pull/616#issuecomment-2221064357

Nishchit14 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

icflorescu / mantine-datatable