Skip to content

Conversation

amanmahajan7
Copy link
Collaborator

No description provided.

@amanmahajan7 amanmahajan7 self-assigned this Apr 20, 2020
@amanmahajan7 amanmahajan7 requested review from nstepien and qili26 and removed request for nstepien April 20, 2020 19:29
@amanmahajan7 amanmahajan7 marked this pull request as ready for review April 20, 2020 19:29

const reorderedColumns = columns.map(c => {
if (c === sourceColumn) return targetColumn;
if (c === targetColumn) return sourceColumn;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it make more sense to move the dragged column before the target column?
I think the usual behavior when dragging things is to move them around rather than swap two objects around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, I will update it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

amanmahajan7 and others added 4 commits April 20, 2020 15:06
Co-Authored-By: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
Co-Authored-By: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
nstepien
nstepien previously approved these changes Apr 20, 2020
Co-Authored-By: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
@nstepien nstepien merged commit fd15c62 into canary Apr 20, 2020
@nstepien nstepien deleted the am-draggable branch April 20, 2020 20:17
@@ -0,0 +1,57 @@
import React from 'react';
Copy link
Contributor

@qili26 qili26 Apr 21, 2020

Choose a reason for hiding this comment

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

I'm just not sure about this file. What do you guys think if we provide this in the DataGrid and export this as a default DraggableHeaderRenderer?

pros: consumer devs can just use the default column function;
cons: 1. we might need to provide more APIs. 2. We have to include the react-dnd as a dependency...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer to keep the dependencies to a minimum and provide a flexible API so users can write their own implementations. Composition is always more maintainable than adding extra props

Copy link
Collaborator

@nstepien nstepien Apr 21, 2020

Choose a reason for hiding this comment

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

Yes it's extra work to maintain the feature + dependency on other libs.
That way users can use whatever implementation they want to header dragging.

import { useDrag, useDrop, DragObjectWithType } from 'react-dnd';

import { HeaderRendererProps } from '../../../../src';

Copy link
Contributor

Choose a reason for hiding this comment

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

And we don't need this empty line.

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