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

Add columns reordering example #2007

Merged
merged 11 commits into from
Apr 20, 2020
Merged

Add columns reordering example #2007

merged 11 commits into from
Apr 20, 2020

Conversation

amanmahajan7
Copy link
Contributor

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
Contributor

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
Contributor 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
Contributor Author

Choose a reason for hiding this comment

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

stories/demos/ColumnsReordering.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
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
stories/demos/ColumnsReordering.tsx Outdated Show resolved Hide resolved
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
Contributor 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
Contributor

@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.

None yet

3 participants