Enhancement 1244 - Slick Arrows States

[simeydotme]

Yo, @kenwheeler , @herrschuessler 👨 - please have a quick look over this commit, and this jsfiddle http://jsfiddle.net/simeydotme/qdgwc3gp/ to demonstrate the fix :) - should fix #1244 by adding some state classes for the arrows (when the arrows are dom objects) ...

the .scss change, though, seems like it could cause trouble for people, so we can discuss to leave it out, or bump to 1.6.0 ? - There's been a lot of fixes in the last couple of weeks, so it might not be a bad idea.

• fixes Custom Arrows disappear on slickAdd #907
• fixes .slick-disabled class is not added to arrows when there's nothing to slide #1244

Si.

[herrschuessler]

Good idea to use .slick-hidden rather than .slick-disabled for this use case.

Concerning backwards compatibility - as far as semver is concerned, minor version changes as well as patch version changes need to be backwards compatible.

[simeydotme]

hmm, yeh ... well it's possible to save the .scss change for 2.0, then. 🐼

[kenwheeler]

Is this a breaking change? or just the scss part?

[simeydotme]

it's just the scss, without that the JS has no backwards-effect, it jsut adds classes for future use. -- so it's possible to undo the .scss change and document it and wait for 2.0.0 ? :)

[simeydotme]

Any verdict on the CSS, @kenwheeler ? :) am keen to kill this branch before it gets too old.

[nominalaeon]

Not contributing, just trying to understand the codebase better:

so the slick-arrows get created no matter what, but they are hidden in the "else", if the slideCount is less than the slidesToShow, right? I'm not seeing how/where the slick-hidden class could have been added before this point. Does the removeClass('slick-hidden') need to fire in the if condition (lines 449, 450)?

[simeydotme]

@nominalaeon thanks for having a look over!! :) -- its been a while since I committed this, and it seems like you might be correct on first glance... but then I realised it's because of the reinit() method, which is called during a setOption, addSlide and removeSlide call. If a slide is added, or removed, our slideCount increases/decreases, so we need to remove the class if it triggers that condition. Otherwise the arrows would still be hidden!

This is why it's so hard to work on Slick, hehe.... there's a billion options and methods with a billion more scenarios 😨 And just when you think you've nailed it, someone's cat ordered a slider which does backflips while on fire! :P

After reviewing the code you mentioned though, I did find one thing I overlooked:
https://github.com/simeydotme/slick/blob/f4a469407edd5caf7efc8c5d217ba13a1ba461a0/slick/slick.js#L2294-L2299
These lines need to check properly for the htmlExpr regex, because currently if you use the options: { prevArrow: ".myArrow", nextArrow: ".myOtherArrow" } the arrows will be removed from the DOM forever during a setOption or addSlide 😟

[simeydotme]

Just rebased from master and did a git push --force into this branch 😄

[jDavidnet]

what does this line of code do?

centerOffset is not used anywhere else in the function, and it's not part of a closure either.

[simeydotme]

Please keep comments relevant to this delta, and not the core of the library, there are other places to do that where they won't get forgotten in time :)

[jDavidnet]

I think .slick-disabled and .slick-hidden are not state-full/semantic enough descriptors. they seem too close to the CSS you are expecting; it's like naming a class .red { color: red; }.

Semantically the states are more like .slick-arrow-out-of-range and .slick-arrow-infinite.

If options.infinite === true then the .slick-arrow-infinite class would always be set.
If a $prevArrow or $nextArrow were out of range, then .slick-arrow-out-of-range would be set.

that way you could have 4 states, and if you wanted the arrows to have a programatic function to toggle the arrows on and off, you could use .slick-arrow-disabled for when the programatic function disabled the buttons specifically. ie. the button could look different if you are out of range and in infinite mode, and you are going to scroll anyways.

specifically right now, i'm working on an application and the design calls for the arrows to be in a 'disabled' state while the arrow would scroll to out of range content, so they need to be visible, but disabled even though your current class would be .slick-hidden in this PR/example.

you also need to update the updateArrows function so if breakpoints are triggered then the arrows will update with the proper classes.

currently i'm working on a patch in code that can extend the prototype, but i wish there was a better way than this, in situations where i don't want to modify the core library, but want to change how it works. Here is how i am doing it now, but doing it this way I can't modify the prototype until after the lib inits on the elements.

        var Slick = this.$el.find('.carousel-group').get(0).slick;
        extendSlick(Slick);

    function extendSlick(slick){
        instrumentFunction(slick, 'updateArrows');
        instrumentFunction(slick, 'buildArrows');
        instrumentFunction(slick, 'checkResponsive');
        instrumentFunction(slick, 'reinit');
        instrumentFunction(slick, 'init');
        instrumentFunction(slick, 'refresh');
        instrumentFunction(slick, 'resize');
    }

    function instrumentFunction(obj, fnName){
        // validate the input
        if(!(obj && obj.__proto__ && (fnName || '' ).length)){
            console.warn('invalid arguments');
            return;
        }

        var fn = obj.__proto__[fnName];
        obj.__proto__['__' + fnName] = fn;

        obj.__proto__[fnName] = function(){
            console.log('.'+fnName, this, arguments );
            fn.apply(this, arguments);
        }

    }

I might be able to do a PR on the lib to show you how I end up solving it without having to extend the library. I think you can add the other states now, without a huge version number as long as the current states stick around until you are able to version them out.

[simeydotme]

I think .slick-disabled and .slick-hidden are not state-full/semantic enough descriptors. they seem too close to the CSS you are expecting; it's like naming a class .red { color: red; }.

Semantically the states are more like .slick-arrow-out-of-range and .slick-arrow-infinite.

There's a much larger debate here, and I don't think it is relevant to this specific PR; You suggestions are relevant very much so to a 2.0 discussion, but I think in that discussion we should think of better names that slick-arrow-out-of-range or adopt a more SMACSSy approach.

[simeydotme]

you also need to update the updateArrows function so if breakpoints are triggered then the arrows will update with the proper classes.

Not true; have you tested this? -- can you show it in a JSFiddle?

edit: actually; no need... my original JSFiddle shows clearly that is indeed working correctly on breakpoint.

[kenwheeler]

👍