Skip to content

[Feature] Drag column to reorder#702

Merged
malonecj merged 14 commits intoComcast:masterfrom
martinnov92:master
Mar 29, 2017
Merged

[Feature] Drag column to reorder#702
malonecj merged 14 commits intoComcast:masterfrom
martinnov92:master

Conversation

@martinnov92
Copy link
Contributor

@martinnov92 martinnov92 commented Mar 18, 2017

Description

Hello, based on the needs of the company I work for and this issue #103, I decided to implement drag column to reorder.

To make this work you need to specify draggable:true to columns.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines: https://github.com/adazzle/react-data-grid/blob/master/CONTRIBUTING.md
  • Tests for the changes have been added (for bug fixes / features)
    !! I am not exactly a test master, so I think my tests sucks :D if anyone wants to help me with that I would be happy
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

Current behavior is that we can't change order columns by dragging.

What is the new behavior?

You can drag and drop a column to make them change their order

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[X] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

In the example file, when column reorder is being made I had to setState twice, because for some reason the column won't reorder when I try to setState only once.
I had to setState to empty columns first and then update them to new order :/.

Thanks for any feedback and help.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 87168b2 on martinnov92:master into ** on adazzle:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 87168b2 on martinnov92:master into ** on adazzle:master**.

@martinnov92 martinnov92 changed the title Drag column to reorder [Feature] Drag column to reorder Mar 20, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d9a2bac on martinnov92:master into ** on adazzle:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling db0e35f on martinnov92:master into ** on adazzle:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling db0e35f on martinnov92:master into ** on adazzle:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3a3b096 on martinnov92:master into ** on adazzle:master**.

@martinnov92
Copy link
Contributor Author

Here is little clip, where I show you how it works, the second part with popup is not in this pull request, but it is easy to implement it on your own.
https://drive.google.com/file/d/0B04DA-ziLi_GSEJLUnBkMVU3QVU/view

@jpdriver
Copy link
Contributor

hey @martinnov92 -- thanks for such a detailed pull request! video, tests, and an example page 👌

@martinnov92
Copy link
Contributor Author

Hi @jpdriver , I just wanted to know if someone is gonna do a code review on this pull request any time soon? thank you

@jpdriver
Copy link
Contributor

@malonecj would you mind casting your eye over this? looks good to merge to me?

@Sofiane000
Copy link

Sofiane000 commented Mar 23, 2017

@jpdriver, any idea when will this be merged?

@Sofiane000
Copy link

Sofiane000 commented Mar 27, 2017

@jpdriver, we yet to get an answer when would this be merged!!

@malonecj malonecj self-requested a review March 29, 2017 10:19
Copy link
Contributor

@malonecj malonecj left a comment

Choose a reason for hiding this comment

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

HI there. Sorry, to be a bit slow for responding at the moment.

Looks great. Just some minor comments to address


module.exports = exampleWrapper({
WrappedComponent: Example,
exampleName: 'Draggable header',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to 'Drag Columns to Reorder'


// ?????? is there better sollution for this??
this.setState(
Object.assign(this.state, {
Copy link
Contributor

Choose a reason for hiding this comment

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

this will mutate the state directly. It is always a better idea to use Object.assign into an empty object such that a new object is created

Object.assign({},this.state, { columns: []})

Copy link
Contributor Author

@martinnov92 martinnov92 left a comment

Choose a reason for hiding this comment

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

Hello @malonecj , thank you for your code review, I tried to fix the errors and hope I did it right 😄 . Thank you.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bd1608b on martinnov92:master into ** on adazzle:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bd1608b on martinnov92:master into ** on adazzle:master**.

@malonecj malonecj merged commit 058eef3 into Comcast:master Mar 29, 2017
@mpmp0
Copy link

mpmp0 commented Mar 29, 2017

Can anyone give an example with Draggable Row Reordering?

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.

6 participants