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

Bugfix/detach grid #360

Merged
merged 4 commits into from
Feb 24, 2016
Merged

Conversation

radiolips
Copy link
Member

Fixes issue in new code found by @jhpedemonte (thanks!).

radiolips added a commit that referenced this pull request Feb 24, 2016
@radiolips radiolips merged commit bc62227 into gridstack:master Feb 24, 2016
@jhpedemonte
Copy link

This isn't fully working with latest code. Nodes are still deleted from DOM when doing a destroy(false).

It works fine with commit 9c01f02 (if I change removeAll(!0) to removeAll(!1) in gridstack.min.js) but fails with commit 85740de (& master). Trying to find out what changed in gridstack.min.js.

@jhpedemonte
Copy link

Hmm, wait, I may have the commits wrong. But definitely still an issue on master. Just trying to find out where things went wrong.

@jhpedemonte
Copy link

Seems to be caused by commit a33f046 (after manually patching gridstack.min.js to removeAll(!1)). Any idea why these changes would still delete nodes when calling destroy(false)?

@radiolips
Copy link
Member Author

Okay, give it one more try. We had found a silly bug a few days ago when we were doing our unit tests. Unfortunately, it turns out that fixing that bug created a new one. I have fixed that. Thanks for your patience and thank you very much for tracking it down to that particular commit. A slight bit of a doozy, but now it should work.

For reference, removeWidget with detachNode=false was also broken. Both destroy as well as removeWidget should now work properly with detach false.

@radiolips radiolips deleted the bugfix/detach-grid branch February 24, 2016 18:25
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.

3 participants