FlexSlider icon indicating copy to clipboard operation
FlexSlider copied to clipboard

itemMargin Carousel Pagination Issue

Open theenoahmason opened this issue 9 years ago • 9 comments

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:

  • Large slides with small margins work fine. (Not always true.) See "Updates" Below.
  • Small slides with large margins will skip however many slides fit within the cumulative itemMargin of a single flex-viewport.

To Recreate:

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.)

Hacks:

  • ~~Set itemMargin to "0," and use CSS padding & box-sizing:border-box; on carousels' ul.slides > li instead.~~ update: The above does not work, flexslider calculates padding on ul.slides > li regardless of box-sizing CSS.
  • Set itemMargin to "0," and use CSS padding & box-sizing:border-box; on carousels' ul.slides > li img or whatever container is INSIDE ul.slides > li. This will, however, "shrink" the slides essentially, not move them over, so style accordingly. Theoretically, I would imagine this hack would be immune to whatever changes are made to fix this itemMargin pagination issue.

Updates:

01/06/2016

  • It seems that pagination on carousels is completely ignoring itemMargin. Issue exists any time the cumulative itemMargin in a flex-viewport is larger than the itemWidth. All carousels with itemMargin set to "0" work as intended.
  • Issue seems to be restricted to a single flex-viewport. It seems that, on long carousels, the loss of slides DOES NOT increase as pagination progresses.
  • Items in view at "itemMargin: 0" determine how many slides are skipped, regardless of the itemMargin set. The number of total pages does not increase as a factor of itemMargin. Example: If 7 slides fit inside the carousel's viewport with the itemMargin set to "0," then pagination will always skip that amount of slides, regardless of the itemMargin. This is bad because their spacing relative to flex-viewport has now changed given the extra margins; Extra pages (dots) need to be added to compensate. To put it in other words, if 7 slides fit in the viewport with itemMargin set to "0," then on first pagination, the carousel will always jump to slide 8, EVEN IF only 4 or 5 slides are visible now because of their itemMargin.
  • When using maxItems, the problem is solved, untill slide hits the itemWidth breakpoint, which appears to act as a slide min-width. Problem appears to be isolated to carousels responding to their itemWidth; NOT to resized, responsive slides as set by maxItems. (Unless used *asNavFor another synced slider, Read Below.)

01/07/2016

  • The above maxItems fix does NOT solve the issue when using carousel asNavFor another synced slider. The left/right pagination on the navigation carousel seems to work, however the same issue as described in this thread still occurs when navigating using the MAIN SYNCED slider's left/right navigation. "if 7 slides fit in the viewport with itemMargin set to "0," then on first pagination, the carousel will always jump to slide 8, EVEN IF only 4 or 5 slides are visible now because of their itemMargin." This causes extremely unpredictable behavior when using maxItems on a carousel that is set to asNavFor another synced slider.

theenoahmason avatar Jan 07 '16 00:01 theenoahmason

any updates here?

theenoahmason avatar Nov 10 '16 02:11 theenoahmason

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 avatar Nov 28 '16 17:11 RwwL

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 avatar Nov 21 '17 20:11 RwwL

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.

RwwL avatar Dec 04 '17 15:12 RwwL

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 avatar Apr 27 '20 15:04 KZeni

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 avatar Apr 27 '20 18:04 KZeni

@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

KZeni avatar Apr 28 '20 16:04 KZeni

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

belcherj avatar May 11 '20 15:05 belcherj

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.

KZeni avatar May 11 '20 16:05 KZeni