Skip to content

fix for #451#910

Closed
andrei-gheorghiu wants to merge 1 commit intogridstack:developfrom
kmap-io:develop
Closed

fix for #451#910
andrei-gheorghiu wants to merge 1 commit intogridstack:developfrom
kmap-io:develop

Conversation

@andrei-gheorghiu
Copy link
Copy Markdown

Description

It seems to fix #451 but, since I'm new to Gridstack, I'd like someone with more intimate knowledge looking over proposed changes and figure out if it doesn't have any unwanted implications.

I'm calling cleanNodes() inside onEndMoving() so when onStartMoving() is triggered on another gridstack container, current gridstack item doesn't get deleted.
This change seems to trigger errors when trying to drag an item in between two different grid containers so I had to add a condition on gridstack.js:731.

Checklist

  • Created tests which fail without the change (if possible)
  • All tests passing (npm test)
  • Extended the README / documentation, if necessary

Note

For my tests, I have upped two.html dependencies to:

  • Bootstrap (all) 3.2.0 > 3.3.7
  • jQuery 1.11.1 > 3.3.1
  • jQueryUI 1.11.0 > 1.12.1
  • lodash 4.17.0 > 4.17.10
  • Gridstack (all) 1.0.0-dev

I haven't committed changes made to two.html into this pull request but I'm ready to, if deemed worthy.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 65.249% when pulling c779873 on kmap-io:develop into 7d9ac26 on gridstack:develop.

@andrei-gheorghiu
Copy link
Copy Markdown
Author

andrei-gheorghiu commented Jun 18, 2018

I'd like to point out the above fix doesn't fix all cases of items disappearing, only the ones caused by moving alternatively items from different containers. It's still possible to delete components by dropping other components on top of them, from same container.

While trying to fix this, I couldn't help but notice listeners stacking on items, which I believe to be the root of the problem. This lib would probably benefit a lot from an overhaul of the events binding and unbinding by either:

a) always making sure a binding is not done twice,
b) providing a method to clean and remap bindings on event (which is pretty much a special case of a). or ...
c) bind on containers, only once, rather than on items (also special case of a)).

Cheers.

@radiolips
Copy link
Copy Markdown
Member

@andrei-gheorghiu Thanks for that note. I believe you're correct about the duplication., and I agree that everything needs to be checked again with those events. We're working through removing jQuery, anyway.

@radiolips
Copy link
Copy Markdown
Member

Closing this as I believe changes in the last several months (including one from this morning) have resolved the referenced issue. Thank you for putting in this PR! Apologies for not getting to it sooner.

@radiolips radiolips closed this Nov 17, 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.

Dragging between gridstack issues: Disappearing gridstack-item, duplicate gridstack-item, resizing gridstack-item.

3 participants