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 2 #361

Merged
merged 3 commits into from
Feb 24, 2016
Merged

Conversation

radiolips
Copy link
Member

Fix one bug and another pops up. This fixes issue where passing detachNode=false to removeWidget would still cause it to be removed from the DOM.

radiolips added a commit that referenced this pull request Feb 24, 2016
@radiolips radiolips merged commit f96dae5 into gridstack:master Feb 24, 2016
@radiolips radiolips deleted the bugfix/detach-grid-2 branch February 24, 2016 18:27
@jhpedemonte
Copy link

Thanks! This is working as expected now.

@jhpedemonte
Copy link

Well, found another issue!

In our project, when we hide a cell, we call removeWidget(w, false). Because we pass false, we don't call _notify, which means we don't call the onchange callback here. That's what resets the position of widgets after one has been removed.

This used to work for our project fine with Gridstack 0.2.4. But now, any widgets below the one we deleted don't get moved up automatically (everything else, including container height, seems to still be set correctly).

How should we handle this? Is there still a defect in Gridstack code, or do we need to call another function/event in order to get things working as expected?

[Our code that handles "hiding" (i.e. removing) widgets is here.]

@jhpedemonte
Copy link

I can workaround this by calling batchUpdate() before removeWidget(w, false), then calling commit() inside of the "change" event listener. That makes things work as before, but I don't think that's a good solution.

@radiolips
Copy link
Member Author

I can update _notify to take in an additional parameter, but it's going to need to be a bigger change and will require updating our unit testing (_notify is unit tested!). I'll try to get to it today, but it may have to wait until tomorrow. I will certainly fix it, though. Thanks for being a trooper through this! Cascading bug fix->new bug has been a pain for you, I'm sure. We should have all of the unit tests written in the next couple weeks to avoid this in the future.

@jhpedemonte
Copy link

No worries. Like I said, we already have this working with v0.2.4, albeit by monkey patching the destroy method. I also don't need this ASAP, so need to hurry.

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.

None yet

3 participants