Removed iCheck dependency

[bobvandevijver]

For as far as I can see, there was only one place where the iCheck was used, which was also in fault.

This PR removes the complete iCheck dependency, as I believe it is not necessary. @sescandell, you were the first to add the iCheck to the bower.json, what's your opinion?

[sescandell]

Actually, iCheck is an AdminLTE's dependency. That is thanks to this plugin checkboxes are rendered with a better look in all templates. Maybe there is some places it is mal-initialized and not working properly?

[bobvandevijver]

@sescandell I do not use the AdminLTE templates, but when I search this repository I do not find any iCheck call to initialize the checkboxes.

In the AdminLTE changelog it states that the iCheck dependency is removed? AdminLTE changelog. Also I can not find any .iCheck() call in the AdminLTE repo... But they do mention in their documentation that they use it.

I will change this and check for the existence of the function before calling, that should provide enough compatibilty :)

[bobvandevijver]

@sescandell How about this?

[sescandell]

Actually, they do use it:

• On the login page for example: https://almsaeedstudio.com/themes/AdminLTE/pages/examples/login.html
• Dependency is in the plugin directory in their repo: https://github.com/almasaeed2010/AdminLTE/tree/master/plugins/iCheck
• And initialization is automatically made on all checkboxes by default.

I'm not sure change event is working... did you had any chance to test it? Otherwise, I'll do on the demo application.

Thanks,

[bobvandevijver]

I did not check the plugins directory, just the bower.json: in my opinion copied dependencies are not the way to go... But okay, I see it's used.

The change event is the default event in jQuery, so I've no reason to suspect it's not working. It is working in my environment, but I did not test it on demo.

Looks like there was a problem with the change event not firing (icheck#244), but it should be solved (icheck#259).

[sescandell]

Great so 👍

[bobvandevijver]

@sescandell Did you test it?

[sescandell]

Not yet,

[sescandell]

Going to test it now,

[sescandell]

Well, actually I just check the previous version and... it's already not working. The reason is the dist version of Adminlte doesn't include the icheck initialization code (found in adminlte/js/app.js but not in admin-lte/dist/js/app.js).

Missing code:

    /*
     * We are gonna initialize all checkbox and radio inputs to
     * ckck plugin in.
     * You can find the documentation at http://fronteed.com/iCheck/
     */
    $("input[type='checkbox']:not(.simple), input[type='radio']:not(.simple)").iCheck({
        checkboxClass: 'icheckbox_minimal',
        radioClass: 'iradio_minimal'
    });

I totally agree with you: AdminLTE doesn't handle frontend dependencies the right way.

[sescandell]

That said, I tried your fix with the iCheck plugin correctly initialized, and it breaks the toggle selector on lists.

[ioleo]

@sescandell maybe we should open an issue (or PR) in AdminLTE project about incorrect handleing of dependencies?

[bobvandevijver]

Guess it's to much work for us to fix the complete AdminLTE dependency handling ;)

@sescandell Do you have any info on why it breaks when the iCheck is initialized?

[sescandell]

@sescandell Do you have any info on why it breaks when the iCheck is initialized?

If I'm not wrong, this is because the change event is not triggered when iCheck is enabled on the concerned checkbox. That's why ifcheck and ifuncheck where binded. Maybe with change ifcheck ifuncheck it works... I'll try that when I have 2 minutes.

@sescandell maybe we should open an issue (or PR) in AdminLTE project about incorrect handleing of dependencies?

Someone already tried that, but PR was refused: ColorlibHQ/AdminLTE#548

[bobvandevijver]

If I'm not wrong, this is because the change event is not triggered when iCheck is enabled on the concerned checkbox. That's why ifcheck and ifuncheck where binded. Maybe with change ifcheck ifuncheck it works... I'll try that when I have 2 minutes.

Well, than the comment I made earlier probably did not yet make it into adminLTE...

Looks like there was a problem with the change event not firing (icheck#244), but it should be solved (icheck#259).

Guess adding the check to the event list should solve it, but than it might also be the case that when the iCheck plugin is updated the event is fired twice. However, with the current implementation of the allElementsChangedHandler that should not really matter.

[bobvandevijver]

@sescandell Did you have any chance to test it?

[sescandell]

Checking it now...

[sescandell]

@bobvandevijver I just tested various combinations. Finally, if we want to be coherent with how the bundle should work initially, here is the right main.js file (IMO):

// Custom scripts belonging to Admingenerator
;(function(window, $, undefined){
    ...
    S2A.batchActionsManager = function(options){
        ...
        this.allElementsToggleButton.on('change ifChecked ifUnchecked', this.allElementsChangedHandler.bind(this));
        ...
    };

    // Called once the DOM is loaded
    $(function(){
        // Initializing the iCheck dependency: should be made in AdminLTE dist app.js code...
        $("input[type='checkbox']:not(.simple), input[type='radio']:not(.simple)").iCheck({
            checkboxClass: 'icheckbox_minimal',
            radioClass: 'iradio_minimal'
        });
        $("input[type='checkbox']:not(.simple), input[type='radio']:not(.simple)").on('ifChecked ifUnchecked', function(){
                $(this).trigger('change');
            });
    });
})(window, jQuery);

That way:

• iCheck is correctly initialized
• users can simply bind on change event to do what they want

Is that good for you?

[bobvandevijver]

@sescandell I'm not sure about this, but I believe we somewhere earlier said that this project should be usable without the AdminLTE dependency as it is not necessary (I do not use it for instance 😄).

So, I took your sample and adjusted it such that it checks if the iCheck functionality is available. I've added it and squashed the PR.

This solution means that it has another check, but in general that should not hurt anyone (actually it helps me, as otherwise the script fails). Furthermore, I've moved more of the initialization calls into the $(function(){}), such that they are also loaded when they need to be (no longer the need to put it in the bottom of the DOM).

I believe in this way we help every user, so, what do you think?

[sescandell]

I believe we somewhere earlier said that this project should be usable without the AdminLTE dependency as it is not necessary (I do not use it for instance 😄).

I don't know where did you read that, but, AFAIK, that's not true. Removing the AdminLTE dependency would break many things: design, menu, navigation, and probably others. I agree we can use the bundle without the AdminLTE, but removing JS / CSS from AdminLTE would break many things.

That said, regarding the PR: 👍

[bobvandevijver]

Guess it's a design choice, but I found the response I was referring to (from @loostro in #245 (comment) about the KnpMenuBundle)

I'm 👍 for removeing the dependency. A dashboard/links system is not the "core" of this bundle. The core of this bundle is generateing CRUD for models, and auto-generateing routes is what I still consider "core", but the menu system is purely optional. It should be described how to quickly setup basic menu system with KnpMenuBundle, but it's not mandatory to use it.

So, I would say that the AdminLTE should be available if you want to use it, but we should not force it (and make it a dependency). I also doubt a lot of stuff would break, but it doesn't matter for now.

I'm merging it and releasing 2.1, thanks for the feedback 😃