[Feature] Issue 245 - Variable Image Width #877

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@wells

wells commented Oct 16, 2013

@mattyza, What do you think?

This pull request solves Issue #245

DEMO
http://jsfiddle.net/U2Ysh/15/embedded/result/

What This Does
I've added a new variable to include in FlexSlider called variableImageWidth.

It causes the slideshow to first size all slides to the viewport width (as per normal). It then determines the minimum image height of the slides (assumption: one image/video per slide),

Required Slider Format:

  • Horizontal
  • Slide Animation
  • Not A Carousel
  • Non-Image/Video Slides are allowed (assumption that the width of these slides is slider.computedW)

The code checks for these requirements before enabling this feature variableImageWidth = slider.vars.variableImageWidth && !vertical && !fade && !carousel

Touch support, animation looping, and variable offsets are a part of this pull request. Meaning it all still works when this feature is enabled.

Note:

I haven't minified the JS, but I can add that into the pull request if you like it. I can also make changes to the README.md and add a little bit of CSS if you like.

Example Slide Image CSS For Variable Width Images

.flexslider .slides img {
    width: auto !important;
    height: inherit;
    max-width: 100%;
    max-height: 100%;
}

Tested so far with:

  • jQuery v1.7.2
  • iPhone 5 (Safari/Chrome)
  • Firefox 24.0
  • Chrome 30.0.1599.69
  • IE 9
@cfxd

This comment has been minimized.

Show comment
Hide comment
@cfxd

cfxd Oct 16, 2013

@wells can you post a jsfiddle demo?

cfxd commented Oct 16, 2013

@wells can you post a jsfiddle demo?

@wells

This comment has been minimized.

Show comment
Hide comment
@wells

wells Oct 16, 2013

@cfxd No problem.

You can even have variable height images as well. I should also mention it is still responsive too.

On thing buggin' me was the viewport CSS in the flexslider default skin transitions everything. I've overwritten that in the fiddle.

http://jsfiddle.net/U2Ysh/15/embedded/result/

wells commented Oct 16, 2013

@cfxd No problem.

You can even have variable height images as well. I should also mention it is still responsive too.

On thing buggin' me was the viewport CSS in the flexslider default skin transitions everything. I've overwritten that in the fiddle.

http://jsfiddle.net/U2Ysh/15/embedded/result/

@cfxd

This comment has been minimized.

Show comment
Hide comment
@cfxd

cfxd Oct 16, 2013

@wells demo fails

cfxd commented Oct 16, 2013

@wells demo fails

@wells

This comment has been minimized.

Show comment
Hide comment
@wells

wells Oct 16, 2013

My guess is that I need to host the javascript on a cdn somewhere... I've embedded the flexslider code.

It should work now at:
http://jsfiddle.net/U2Ysh/15/embedded/result/

wells commented Oct 16, 2013

My guess is that I need to host the javascript on a cdn somewhere... I've embedded the flexslider code.

It should work now at:
http://jsfiddle.net/U2Ysh/15/embedded/result/

@cfxd

This comment has been minimized.

Show comment
Hide comment
@cfxd

cfxd Oct 16, 2013

@wells, well done! Hope your pull request is accepted! I just might use this for production too.

cfxd commented Oct 16, 2013

@wells, well done! Hope your pull request is accepted! I just might use this for production too.

@risottto

This comment has been minimized.

Show comment
Hide comment
@risottto

risottto Oct 27, 2013

Awesome, I set up the slider with variable widths copying the jsfiddle code and it works great, only issue is that the slideshow no longer starts! Trying to find the problem...

Awesome, I set up the slider with variable widths copying the jsfiddle code and it works great, only issue is that the slideshow no longer starts! Trying to find the problem...

@tchopshop

This comment has been minimized.

Show comment
Hide comment
@tchopshop

tchopshop Nov 10, 2013

Wow, this is amazing, exactly what I was looking for.

Wow, this is amazing, exactly what I was looking for.

@illume

This comment has been minimized.

Show comment
Hide comment
@illume

illume Nov 19, 2013

If you don't have images as children... you can change line 859 to this:
var imgWidth = $(this).children().first().width();

I guess there is no need to assume the contents of the slides will be images? How about just checking the width of the slide(or width of children in the slide)?

illume commented Nov 19, 2013

If you don't have images as children... you can change line 859 to this:
var imgWidth = $(this).children().first().width();

I guess there is no need to assume the contents of the slides will be images? How about just checking the width of the slide(or width of children in the slide)?

@carasmo

This comment has been minimized.

Show comment
Hide comment
@carasmo

carasmo Nov 23, 2013

I really love this idea but if the slide image is inside a link it doesn't work. You can't put anything between the li except an image, or the video example. But what if you want that image to link to something or use some light box?

carasmo commented Nov 23, 2013

I really love this idea but if the slide image is inside a link it doesn't work. You can't put anything between the li except an image, or the video example. But what if you want that image to link to something or use some light box?

@sebstck

This comment has been minimized.

Show comment
Hide comment
@sebstck

sebstck Nov 29, 2013

@carasmo
ugly hack: load the image but dont display it so the script can get the proper width,
add linked your image afterwards

http://jsfiddle.net/sebstck/hcHBU/

sebstck commented Nov 29, 2013

@carasmo
ugly hack: load the image but dont display it so the script can get the proper width,
add linked your image afterwards

http://jsfiddle.net/sebstck/hcHBU/

@Twansparant

This comment has been minimized.

Show comment
Hide comment
@Twansparant

Twansparant Dec 9, 2013

@sebstck:
I would like to achieve the same thing as carasmo, but your link goes to the default jsfiddle without image linking?

@sebstck:
I would like to achieve the same thing as carasmo, but your link goes to the default jsfiddle without image linking?

@Twansparant

This comment has been minimized.

Show comment
Hide comment
@Twansparant

Twansparant Dec 9, 2013

Thanks sebstck, I got this working too!
One thing I don't understand is why you don't use the itemMargin parameter as marginitem variable:
itemMargin = slider.vars.itemMargin, to get the margin-right working properly.

@risottto: Just remove the slideshow: false, line

Thanks sebstck, I got this working too!
One thing I don't understand is why you don't use the itemMargin parameter as marginitem variable:
itemMargin = slider.vars.itemMargin, to get the margin-right working properly.

@risottto: Just remove the slideshow: false, line

@andyknapp

This comment has been minimized.

Show comment
Hide comment
@andyknapp

andyknapp Feb 18, 2014

This is great! Thanks @wells

This is great! Thanks @wells

@wells

This comment has been minimized.

Show comment
Hide comment
@wells

wells Mar 5, 2014

No worries @andyknapp

I did find a bug with Chrome mobile for iOS, The slide offset appears to be off by one at times. I don't believe this is from my additional code.

@Twansparant @sebstck good catch on the margins.

Just FYI, I think I'll actually write a new slider plugin, with cleaner code, that is capable of variable width image/video magic, runs more efficiently, margins, centers images smaller than the viewport, and has variable clones (for when overflow is no longer hidden). I'll also fix up the touch interface so that mobile page scrolling is smoother. I'll let you know when this is ready. It's on my list of things to do.

Also, I recommend wrapping the slider code in a $(window).load(function() { });

wells commented Mar 5, 2014

No worries @andyknapp

I did find a bug with Chrome mobile for iOS, The slide offset appears to be off by one at times. I don't believe this is from my additional code.

@Twansparant @sebstck good catch on the margins.

Just FYI, I think I'll actually write a new slider plugin, with cleaner code, that is capable of variable width image/video magic, runs more efficiently, margins, centers images smaller than the viewport, and has variable clones (for when overflow is no longer hidden). I'll also fix up the touch interface so that mobile page scrolling is smoother. I'll let you know when this is ready. It's on my list of things to do.

Also, I recommend wrapping the slider code in a $(window).load(function() { });

@klihelp

This comment has been minimized.

Show comment
Hide comment

klihelp commented Apr 19, 2014

+1

@loayza

This comment has been minimized.

Show comment
Hide comment
@loayza

loayza Jul 3, 2014

  • edited message -

Hi!

Very good work!

I'm working in a project with FlexSlider and I needed an updated version of the plug-in but with this commit included.

It seems that the pull request you did before got stucked due a merge conflict.

So I've finally managed to make a fork of the current FlexSlider and make a rebase with your commit.

If you don't mind I will make a pull request of my fork and so your commit will be sent again with it.

Please let me know if you don't agree with it. My only intention is to add your code to the official plug-in :-)

Anyways, if they don't accept the request I will try to maintain my fork up to date with the official version.

Kind regards.

loayza commented on c818006 Jul 3, 2014

  • edited message -

Hi!

Very good work!

I'm working in a project with FlexSlider and I needed an updated version of the plug-in but with this commit included.

It seems that the pull request you did before got stucked due a merge conflict.

So I've finally managed to make a fork of the current FlexSlider and make a rebase with your commit.

If you don't mind I will make a pull request of my fork and so your commit will be sent again with it.

Please let me know if you don't agree with it. My only intention is to add your code to the official plug-in :-)

Anyways, if they don't accept the request I will try to maintain my fork up to date with the official version.

Kind regards.

This comment has been minimized.

Show comment
Hide comment
@wells

wells Jul 10, 2014

Owner

Go ahead. They don't seem to maintain this plugin very well.

Owner

wells replied Jul 10, 2014

Go ahead. They don't seem to maintain this plugin very well.

@zanematthew

This comment has been minimized.

Show comment
Hide comment
@zanematthew

zanematthew Jan 26, 2015

+1 This really should be merged

+1 This really should be merged

@wells

This comment has been minimized.

Show comment
Hide comment
@wells

wells Jan 28, 2015

Thanks to @loayza for adding the commit to the latest version. I agree, it should be merged.

wells commented Jan 28, 2015

Thanks to @loayza for adding the commit to the latest version. I agree, it should be merged.

@zanematthew

This comment has been minimized.

Show comment
Hide comment
@zanematthew

zanematthew Feb 3, 2015

@wells I did notice a possible bug, it seems that when using the params asNavFor, and sync to display image thumbnails the "active" class is no longer applied.

@wells I did notice a possible bug, it seems that when using the params asNavFor, and sync to display image thumbnails the "active" class is no longer applied.

@wells

This comment has been minimized.

Show comment
Hide comment
@wells

wells Feb 4, 2015

Yeah, I only built for my use case without thumbs. Given this repo hasn't been touched since June, it is tempting to fork and move on and/or simply start a new slider project.

wells commented Feb 4, 2015

Yeah, I only built for my use case without thumbs. Given this repo hasn't been touched since June, it is tempting to fork and move on and/or simply start a new slider project.

@jeffikus jeffikus closed this Apr 1, 2015

@zanematthew

This comment has been minimized.

Show comment
Hide comment
@zanematthew

zanematthew Apr 1, 2015

Actually I was able to achieve this using plain ol' CSS.

.flexslider.thumbnails .slides > li {
    width: auto !important;
    }

.flexslider.thumbnails img {
    width: auto !important;
    height: inherit;
    max-width: 100%;
    max-height: 60px;
    }

Did you try that? vs. doing it via JS?

Actually I was able to achieve this using plain ol' CSS.

.flexslider.thumbnails .slides > li {
    width: auto !important;
    }

.flexslider.thumbnails img {
    width: auto !important;
    height: inherit;
    max-width: 100%;
    max-height: 60px;
    }

Did you try that? vs. doing it via JS?

@wells wells deleted the wells:feature/variable-image-width branch Apr 9, 2015

@yiikoma

This comment has been minimized.

Show comment
Hide comment
@yiikoma

yiikoma Jul 13, 2015

@wells I came upto a bug which might be interesting for you.

here is your jsfiddle but without the placeholder #6 and without the video. the images have a fixed height of 250px for demonstration purpose:
http://jsfiddle.net/U2Ysh/161/

if you now klick through the slides till the end a long white gap appears. If the last image reached end of flexslider the images appear as excpected for the infinite loop.

Do you know a solution to avoid the white gap and load the images one after the other without being forced to reach the end of flexslider?

yiikoma commented Jul 13, 2015

@wells I came upto a bug which might be interesting for you.

here is your jsfiddle but without the placeholder #6 and without the video. the images have a fixed height of 250px for demonstration purpose:
http://jsfiddle.net/U2Ysh/161/

if you now klick through the slides till the end a long white gap appears. If the last image reached end of flexslider the images appear as excpected for the infinite loop.

Do you know a solution to avoid the white gap and load the images one after the other without being forced to reach the end of flexslider?

@zanematthew

This comment has been minimized.

Show comment
Hide comment
@zanematthew

zanematthew Jul 13, 2015

Maybe just set all images to fade in, via a CSS class and opacity :(

Maybe just set all images to fade in, via a CSS class and opacity :(

@yiikoma

This comment has been minimized.

Show comment
Hide comment
@yiikoma

yiikoma Jul 14, 2015

@zanematthew example fiddle what you mean?

problem is, only the first image of the slider is cloned at the end of the slideshow. i need something like clone the first, second, third, ..., or doTheClone not at the ende of the slider but if the white gap appears.

yiikoma commented Jul 14, 2015

@zanematthew example fiddle what you mean?

problem is, only the first image of the slider is cloned at the end of the slideshow. i need something like clone the first, second, third, ..., or doTheClone not at the ende of the slider but if the white gap appears.

@hellboy81

This comment has been minimized.

Show comment
Hide comment
@hellboy81

hellboy81 Jun 30, 2016

Is this merge request integrated into master branch? I can not find variableImageWidth !

Is this merge request integrated into master branch? I can not find variableImageWidth !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment