dynamicweb / swiffy-slider

Super fast carousel and slider with touch for optimized websites running in modern browsers.
MIT License
250 stars 29 forks source link

Avoid stretched images if image doesn't exactly fit #18

Closed aimeos closed 2 years ago

aimeos commented 2 years ago

When using width and height: swiffyslider-streched

After using max-width and max-height: swiffyslider-ratio

nicped commented 2 years ago

Thank you for your pull request. I have just tested it out and I run into issues with this change.

I.e. without this change: image

And with the change to max-width and max-height: image

I would like to see the markup that you have. Maybe it looks like this:

<div class="slider-container">
  <img scr="...">
</div>

Maybe you just have to wrap your images in a div like this so the div becomes the slide and then limit your image inside the slide:

<div class="slider-container">
    <div><img src="../assets/img/photos/img4.webp" style="max-width: 100%;max-height:100%;"></div>
    <div><img src="../assets/img/movies/movie1.webp" style="max-width: 100%;max-height:100%;"></div>
    <div><img src="../assets/img/photos/img6.webp" style="max-width: 100%;max-height:100%;"></div>
    <div><img src="../assets/img/photos/img7.webp" class="mh-100 mw-100" title="using bootstrap classes instead of style"></div>
</div>

Problem is that if the slide element in it self is not 100%x100% the layout will start to break. The above will work nicely, but will only center vertically and not horisontally.

Below is an alternative - set the slide div to be flex and align items center on both axes and set the image inside to max width and height to 100% and with auto margin. This solution will center the image, keep the slide size and work with all image sizes.

.slider-container>* {
    display: flex;
    align-items: center;
    justify-content: center;
}

.slider-container>*>img {
    max-width: 100%;
    max-height: 100%;
    margin: auto;
}

This approach could be conceptualized and added as an additional css class for Swiffy slider.

Would look like this: image

Let me hear what you think.

aimeos commented 2 years ago

Thanks for your suggestion! Added the additional <div> tag and figured out that using this is enough when combined with slider-item-ratio-contain:

.slider-container>* {
    scroll-snap-align: var(--swiffy-slider-snap-align);
    position: relative;
    display: flex;
    align-items: center;
    justify-content: center;
}

Instead of:

.slider-container>* {
    scroll-snap-align: var(--swiffy-slider-snap-align);
    position: relative;
    width: 100%;
    height: 100%
}
nicped commented 2 years ago

Great, glad you made it work! I will close this pull request then.

Thanks for participating!

aimeos commented 2 years ago

@nicped: The correct code is:

.slider-container>* {
    scroll-snap-align: var(--swiffy-slider-snap-align);
    position: relative;
    display: block;
}

.slider-item-ratio .slider-container>* {
    display: flex;
    align-items: center;
    justify-content: center;
}

Otherwise, full page sliders may not cover the whole width due to display: flex.

nicped commented 2 years ago

Great - thanks!