woocommerce / FlexSlider

An awesome, fully responsive jQuery slider plugin
http://www.woocommerce.com/flexslider/
GNU General Public License v2.0
4.92k stars 1.7k forks source link

itemMargin Carousel Pagination Issue #1480

Open theenoahmason opened 8 years ago

theenoahmason commented 8 years ago

Pertaining to Carousels with itemWidth set and itemMargin set:

ItemMargin calculates ul.slides properly, and adds margin-right properly, however pagination is broken when itemWidth is set to a small number, and itemMargin is set to anything large enough to create cumulative spacing inside flex-viewport that is larger than a slide.

Behavior:

Make a carousel with 20 or so slides, Set itemWidth to "100", and set itemMargin to "90." If the carousel is wide enough, you will see that on pagination, some slides are skipped. The same behavior is present when using carousel asNavFor another synced slider, ruining that behavior as well.

See Issue Here:

I am currently creating a wordpress plugin using flexslider, but have effectively recreated the problem on this latest version (2.6), and the last version of Flexslider2 standalone in a codepen.

Examples use only default markup from flexslider2 examples, default flexslider.css v2.6 stylesheet, jQuery v1.11.3, jquery.flexslider.js v2.6, and the default initialization of flexslider2 examples. (I have set slideshowSpeed to a longer interval to be able to visually see skipped slides.)

01/06/2016

01/07/2016

theenoahmason commented 7 years ago

any updates here?

RwwL commented 7 years ago

I was just about to submit this test case — http://lifford.org/exp/testcase/Flexslider-margin-issue/ — as a new issue, but looks like it would probably be a duplicate of this one. Glad to see this is set up as a milestone item for 2.7.0.

RwwL commented 6 years ago

I decided to try to fix this in our fork and so far it looks like this issue can be fixed (at least partially?) very simply: in the slider.doMath function, in its if (carousel) clause, just change the calculation of slider.visible from this:

slider.visible = Math.floor(slider.w/(slider.itemW));

to this:

slider.visible = Math.floor(slider.w/(slider.itemW + slideMargin));

Will be testing some more but so far this seems to solve the issue without any unexpected side effects — but I haven't set myself up to test the 01/07/2016 update in the issue.

Should I submit this as a pull request? I was hesitant to do so for a one-liner because the team might be annoyed at sifting through our other updates (I added NPM build scripts to minify, the JS, compile the LESS, and copy the new CSS and JS to the appropriate spots in the demo, but I wasn't sure if the team had other plans for how to address that sort of stuff). I think those are super-useful updates though and would be happy to contribute them if the the maintainers want 'em.

RwwL commented 6 years ago

Actually, that single line in my comment above isn't enough, but I've got a more solid, well-tested clause in place for this now and am submitting a pull request.

KZeni commented 4 years ago

How has a bug that can totally throw off pagination (not showing paging/direction navigation elements when it should [items are clearly being cut off & you can't navigate to see them fully], etc.) due to not including margin when checking how many items are visible in a carousel (seems like a fundamental thing to include as well as the item width to know the total space taken up by the current items in the carousel) still not been officially updated with the supplied fix yet?

I encountered this issue just now using the latest FlexSlider version, and it turns out that this has been an issue with a suggested fix for years...?

Seems it might go unnoticed for carousels that have a small amount of margin between the carousel items as it definitely becomes more prominent of an issue when carousels have larger margins between items (it should definitely be updated to work in all cases & it's technically broken for both [just isn't as noticeably broken in the smaller margin setup.])

Also, the workaround of making sure maxItems is supplied isn't readily available for sites using plugins like MetaSlider & others where that setting isn't shown.

Any update on getting this resolved in the next official version so I can use that confidently moving forward without worrying about the fix for this being overwritten (again, by updating to a new release of FlexSlider; updates shouldn't revert bugfixes, ideally) to then have this issue resurface?

KZeni commented 4 years ago

Regarding the proposed fix in the PR...

I might have it where

slider.visible = Math.floor(slider.w/(slider.itemW));

is updated to be

slider.visible = Math.floor(slider.w/(slider.itemW + slideMargin));

instead actually be changed to

slider.visible = slider.itemW > 0 && slider.itemM > 0 ? Math.floor(slider.w / (slider.itemW + slider.itemM)) : Math.floor(slider.w / slider.itemW);

That way, items that don't actually have these values to calculate still use the old/basic setup (I was encountering a browser hang/oddities without that present with multiple sliders on page [some carousels & some not] with some not shown immediately while other are.)

KZeni commented 4 years ago

@jeffikus Any update or thoughts on what I've proposed (also sent via email; this bug really should be fixed)?

Could @belcherj and/or possibly others help if @jeffikus isn't available?

Thanks, Kurt

belcherj commented 4 years ago

I am not involved in this project. Sorry I could not be of help.

KZeni commented 4 years ago

FYI, https://github.com/DavidAnderson684/FlexSlider/pull/6 has been implemented (PR merged) to fix this via the fork of FlexSlider being used in the MetaSlider WordPress plugin. Still think it'd be worthwhile to be resolved directly in FlexSlider itself, though, and figured I might as well share what MetaSlider ended up doing to resolve this issue.