Accessible360 / accessible-slick

the last (accessible) carousel you'll ever need.
https://accessible360.github.io/accessible-slick
MIT License
254 stars 45 forks source link

Issue with backwards compatibility with original slick slider #57

Closed warriotox closed 2 years ago

warriotox commented 2 years ago

Hello, I am trying to replace slick slider 1.8.0 in a project and so far it seems the JS is 100% backwards compatible so great job there! But the HTML is not. With slick slider each immediate child of the slick container became a .slick-slide, but with accessible slick each immadiate child becomes a child of a child of a .slick-slide on init. What is up with that? Is there any accessibility reasoning behind this? I am wondering if I should rewrite the accessible-slick JS and remove this logic, but if it will break some accessibility then I won't...

With slick slider: <div class="slick-slider"><div class="custom-class">text</div></div> becomes

<div class="slick-slider">
  <div class="slick-list">
    <div class="slick-track">
      <div class="slick-slide custom-class">text</div>
    </div>
  </div>
</div>

With accessible slick: <div class="slick-slider"><div class="custom-class">text</div></div> becomes

<div class="slick-slider">
  <div class="slick-list">
    <div class="slick-track">
      <div class="slick-slide">
        <div>
          <div class="custom-class" style="width:100%;display:inline-block">text</div>
        </div>
      </div>
    </div>
  </div>
</div>

Additionally, the original item gets inline styles for width and display. The width was there even in slick-slider, but it was set on the .slick-slide. Why are these inline styles set on the child item, like why force the div to be display: inline-block and not just leave it as block?

warriotox commented 2 years ago

I have found out that this behavior is the result of the slidesPerRow logic. This piece of code nests the original HTML inside two extra div containerers. See source code below...

For some reason my old slick 1.8.0 has this exact same code, but it doesn't trigger, never, ever. It always behaves as if the slidesPerRow options was set to 0...

I didn't even know this was a supported option until today and because I am trying to make the upgrade backwards compatible I have decided to change the default slidesPerRow value from 1 to 0 which will result in the behavior that we are used to in our project (while still making it possible to use this feature if needed). I will close this and leave it here in case anyone else runs into this in the future.

for(a = 0; a < numOfSlides; a++){
    var slide = document.createElement('div'); //first extra div
    for(b = 0; b < _.options.rows; b++) {
        var row = document.createElement('div'); //second extra div
        for(c = 0; c < _.options.slidesPerRow; c++) {
            var target = (a * slidesPerSection + ((b * _.options.slidesPerRow) + c));
            if (originalSlides.get(target)) {
                row.appendChild(originalSlides.get(target));
            }
        }
        slide.appendChild(row);
    }
    newSlides.appendChild(slide);
}

_.$slider.empty().append(newSlides);

_.$slider.children().children().children()
    .css({
        'width':(100 / _.options.slidesPerRow) + '%', //the extra width inline css
        'display': 'inline-block' //the extra display inline css
    });
PapaGrande commented 1 year ago

I found that setting rows to 0 removed the extra HTML. https://github.com/kenwheeler/slick#settings doesn't explicitly say so, but it effectively turns off the grid mode.