RamonSmit / Nestable2

New pickup of Nestable!
Other
322 stars 147 forks source link

Add some features for remove items. #80

Closed RomanBurunkov closed 7 years ago

RomanBurunkov commented 7 years ago

Hi there,

I've added removeAll method and callback for remove method.

Also I've added true option for jq clone at restoreItemAtIndex method, that helps to save added events for elements inside items.

Please review. Thanks, Roman.

RomanBurunkov commented 7 years ago

Hi folks,

Any body here ;) ?

RamonSmit commented 7 years ago

I'm here, not sure if @pjona is here?

pjona commented 7 years ago

Hi @RomanBurunkov, sorry for delay in answer. I want to review your changes in this week. This PR fixing issue #76, right?

RomanBurunkov commented 7 years ago

Hi @pjona,

No problem. Yes, you are right it adds callback after remove animation as asked in #76.

pjona commented 7 years ago

@RomanBurunkov

I review the code and it works. The only problem is that I'm not big fan of functions with more than 4-5 arguments but I can accept it if @RamonSmit don't see anything wrong with it. I mean this code:

$('#nestable').nestable('remove', 5, 'fade', 'slow', function() {});

Maybe we can move arguments: 'fade' and 'slow' to the global options:

$('#nestable').nestable({ effect: 'fade', effectTime: 'slow' });

Then we can use it also in the removeAll method. (And maybe in the future also when adding new item). From my point of view it's always good to have consistent UI so if you have some effect when removing item it will be good to have this some effect when adding new item - so it's not needed to customise it per method. Then we have 3 arguments:

$('#nestable').nestable('remove', 5, function() {});
RomanBurunkov commented 7 years ago

@pjona

That's make sence. Thanks for review and recommendations.

I'm going to try your approach. I will update this pr when rewrote the code.

Thanks, Roman.

RomanBurunkov commented 7 years ago

Hi @pjona ,

I have got some free time and I've added changed that we discussed previously.

Thanks, Roman.

pjona commented 7 years ago

@RomanBurunkov Really good job! Thanks.

This changes break backward compatibility, I will make new release in few days.