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

Grid support #86

Merged
merged 12 commits into from
Nov 27, 2016
Merged

Grid support #86

merged 12 commits into from
Nov 27, 2016

Conversation

richmeij
Copy link
Collaborator

Added drag & drop support in a grid, as requested in #4.

  • I added a new axis option: xy. I kept this a string to stay backwards compatible. You mentioned it should probably be an array/object, and while I found no immediate need for that, that should be a fairly straightforward change.

  • As you now have the option to (potentially) move over the x and y axis, I had to change most functions to explicitly work with x/y, width/height and top/left instead of using dimension and edge. This resulted in slightly more verbose code, especially in animateNodes(), but I decided to keep it like this initially, so the changes are clear. We can always apply extra changes to make things more compact, if needed.
    I did make an effort to keep loops and calculations as effecient as possible.

Other changes:

  • Added Grid story
  • Added some missing proptypes in stories

@clauderic
Copy link
Owner

Hey @richmeij!

First of all, a massive thank you for working on this 🙏

I haven't had too much time to review the code yet, but I played around with the storybook and this looks really, really cool.

I'll provide more detailed feedback once I've had the chance to dig deeper 😄

@richmeij
Copy link
Collaborator Author

Glad to help out, @clauderic!

One thing that just occurred to me is that I'm unsure the current approach will play nice with React Virtualized and such. I'll have a look at that in the meantime.

@clauderic
Copy link
Owner

I believe it should play nice with windowing libraries based on your implementation, but don't quote me on that 😅

Two small things off the top of my mind:

  • When testing with items of varying widths, the translate offsets appear to be buggy. Haven't had the change to dig deeper though.
  • I don't believe margins are currently being taken into account. I know, vertical margins can be extremely tricky because of margin collapsing, so perhaps that might end up being a known/documented limitation (i.e., use padding instead).

@richmeij
Copy link
Collaborator Author

richmeij commented Nov 14, 2016

Ok, I will have a look!

Question:
When you mention varying widths, do you mean in a grid setup? Or also, for example in a horizontal list (so basically a regression?)

@richmeij richmeij closed this Nov 14, 2016
@richmeij richmeij reopened this Nov 14, 2016
@clauderic
Copy link
Owner

I mean in a grid. Something like this basicaly: http://greensock.com/?example=sortable-grid-using-draggable

@richmeij
Copy link
Collaborator Author

Yeah, that wouldnt be something that this PR accounts for yet. I'll have a look.

@ingro
Copy link

ingro commented Nov 16, 2016

This will be a very cool feature, i'm actually use react-dnd to sort a grid container but will make the switch to sortable-hoc as soon as this feature land! :)

@tecnoloco
Copy link

tecnoloco commented Nov 18, 2016

Is this feature available? I tried using axis='xy' and I got the next warning message :

Failed prop type: Invalid prop axis of value xy supplied to SortableList, expected one of ["x","y"].

Obviously it didn't work and could not sort any items
I'm using version 0.2.0

@clauderic
Copy link
Owner

@tecnolocoEdan It hasn't been merged yet, no. But feel free to clone the repo locally from this branch if you wish to play around with it ahead of time.

@tecnoloco
Copy link

@clauderic thanks for your response I really interested in this feature working in these awesome hoc's, i tried using this branch inside my project but i couldn't make it work.

@richmeij is there any chance you could make an npm installable version of your branch? i tried to install it a few times but it seems that the src files weren't downloaded, also tried to take the files directly from the zip and didn't work either and no error message was displayed.

@richmeij
Copy link
Collaborator Author

@tecnolocoEdan Strange, it should just work like the master branch. Are you running npm run storybook?

@clauderic
Copy link
Owner

clauderic commented Nov 21, 2016

@tecnolocoEdan, you'll need to run the following commands manually:

git clone https://github.com/richmeij/react-sortable-hoc.git -b grid-support
cd react-sortable-hoc
npm install
npm run storybook

If you want to create a custom build, just run:
npm run build

This is something I'll be working on automating soon.

@tecnoloco
Copy link

Again thanks to both of you ( @richmeij & @clauderic ), you are really helpfull. Finally I made it work, it seems that the only thing missing on the branch of grid-support was a react-dom on the pakage.json, I was geting an error of a missing react-dom when I ran npm install, so I installed it manually just with npm install react-dom and I was able to use the es6 path built with npm run build on my project with no problem.

I expect this info will be helpfull to you and others.

@jfrolich
Copy link

Awesome! Works very well! 🙌👏 Is there perhaps a way to publish this version under a beta version number of so? Or how else to get this working in production?

@clauderic clauderic merged commit 61c4b9c into clauderic:master Nov 27, 2016
DimitarNestorov pushed a commit to codemotionapps/react-sortable-hoc that referenced this pull request Feb 4, 2019
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