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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: drag placeholder #194

Closed
wants to merge 2 commits into from
Closed

feat: drag placeholder #194

wants to merge 2 commits into from

Conversation

Alexays
Copy link
Contributor

@Alexays Alexays commented Jul 24, 2018 •
edited

I took back what you started and finished the implementation
I didn't test with multiple grids
I also added translateZ(0px) to be able to use z-index otherwise it doesn't work.
Updated dists too

Reviews are welcome 馃帀

@Alexays Alexays force-pushed the drag_placeholder branch 2 times, most recently from e3b3c43 to 8fd1683 Compare July 24, 2018 12:18
@niklasramo
Copy link
Collaborator

@Alexays awesome stuff! I'll review as soon as I can. A lot of changes and pretty big feature 馃檪

@niklasramo niklasramo mentioned this pull request Jul 25, 2018
Closed
@Alexays
Copy link
Contributor Author

Alexays commented Jul 31, 2018 •
edited

Ok so I made some changes I moved dragplaceholder to its own file to follow files tree and fixed a bug when deleting an item 馃槂

@Alexays Alexays force-pushed the drag_placeholder branch 2 times, most recently from ca2ec07 to 5f37c14 Compare September 25, 2018 18:47
@Alexays Alexays closed this Oct 5, 2018
@Alexays Alexays reopened this Oct 5, 2018
@Alexays
Copy link
Contributor Author

Alexays commented Oct 5, 2018

Rebased against master, do you need something before you can merge?

Alexays
Alexays commented Oct 5, 2018
View reviewed changes
src/Item/ItemDragPlaceholder.js

var nextPosition;
var i = 0;
for (; i < nextGrid._items.length; i += 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just here i'm not sure is the best way to do it :/

@niklasramo
Copy link
Collaborator

niklasramo commented Oct 6, 2018 •
edited

@Alexays Yeah, I just need some time to review this stuff, but haven't had the juice for it yet :( Sorry to keep this lingering here...

But there are a few notes after a quick glance:

  • Tests! Also for grid to grid dragging use cases.
  • I'm not totally convinced we need the translateZ addition.
  • This feature need to be optional (grid options) and by default off.

@niklasramo
Copy link
Collaborator

niklasramo commented Nov 6, 2018 •
edited

@Alexays I took a closer look at this during weekend and noticed that the scenario of dragging from a grid to another grid is not covered in this implementation, so that's one thing that still needs to be done. Also the .create() and .layout() methods might cause some layout thrashing at the moment so we should definitely batch the DOM reads and writes like the rest of Muuri's code does. And lastly, I think we might want to provide some hooks for the user to take control of the placeholder creation phase so that users can for example use element pooling to avoid garbage collection (at the moment a DOM node is created and removed on every single drag operation). I started my own branch where I try to fix the issues and when it's ready I'll push the code here.

Thanks again @Alexays for the hard work you have done here 鉂わ笍

@flozero
Copy link

flozero commented Dec 6, 2018

i can't wait this feature ^^

@flozero
Copy link

flozero commented Dec 20, 2018

do we have some news about integrating it ?

@niklasramo
Copy link
Collaborator

I'm working on the missing pieces and it's almost there. There are some things still work out (mostly UX stuff), but I'll send the updates as soon as they are ready. It's not as straightforward as it may seem 馃檪

@niklasramo niklasramo mentioned this pull request Dec 20, 2018
Merged
@flozero
Copy link

flozero commented Jan 30, 2019

Cannt wait anymore ^^

@flozero
Copy link

flozero commented Jan 30, 2019

Can you release a beta version ?

@niklasramo
Copy link
Collaborator

Nope, I'll release a fully tested version when it's ready. Unfortunately I haven't had time to spend on muuri's code in the last few months.

@niklasramo niklasramo mentioned this pull request Mar 11, 2019
Merged
@niklasramo
Copy link
Collaborator

niklasramo commented Mar 11, 2019 •
edited

The work started here is continued in #262, getting very close to completion.

@niklasramo
Copy link
Collaborator

Closing this as #262 was merged to dev branch, will be out in the next release.

@niklasramo niklasramo closed this Mar 20, 2019
@flozero
Copy link

flozero commented May 29, 2019

date for the next release ? :D ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Alexays @niklasramo @flozero