Perform current slide calculation based on child elements - #1334
Perform current slide calculation based on child elements#1334indykoning wants to merge 11 commits into
Conversation
| return Math.round( | ||
| (this.sliderSpan / (this.vertical ? this.container.scrollHeight : this.container.scrollWidth)) * this.container.children.length, | ||
| ) |
There was a problem hiding this comment.
The reason we're not calculating from the currentSlide to the last visible slide is because this could cause the visibleSlides to jump up and down depending on your position in the slider.
While visibleSlides is also used in slidesTotal, causing the total to change
There was a problem hiding this comment.
I think this will still cause incorrect calculations for the slidesTotal now. A correct calculation here would take into account first how many items there are, and then only subtract how many items are visible when the slider is all the way at the end.
Is this value still relevant? Because on most shops we already hide the "progress bar" under sliders anyway.
There was a problem hiding this comment.
Where do you see this calculation being incorrect?
Say we have a slider of 7 items where 4 are visible.
The slider is 1252px wide and elements are 313px wide. This would turn into:
1252 / 2191 * 7 = 4
We're taking the visible size, dividing it by the total scroll width (each element including margins, paddings and gaps), and multiplying it by the amount of items in the slide.
Another way you could write it is:
let averageSlideWidth = 2191 / 7; // 313
let visibleSlides = 1252 / 313; // 4The only change in this calculation is that instead of using the width of the first child, we're using the average width of all childs.
I see slidesVisible being used and shown: In slidesTotal, in the "progress bars", the numbers and next and previous buttons where we want the slider to slide pages instead of single slides
There was a problem hiding this comment.
I'm imagining a hypothetical situation where you have 100 wide slides (say, 100px wide each) followed by a bunch extremely tiny slides (say, 1px) at the end, where only two of the wide slides are visible (so the slider span itself is 200px).
slidesVisible would still be 2 even if you have up to 25 of the tiny slides at the end. With the current calculation of the slidesTotal, you'd have 24 extra useless "slides" in your progress bar (as all of the tiny slides would all be visible after slide number 100). Obviously this is an absurd example, but more realistically I could see there being 1 too few or 1 too many slides in real world situations.
| } | ||
| return Math.round(this.position / this.childSpan) % this.slides.length | ||
| // First make a calculated guess, improving performance by not having to loop through all slides | ||
| const bestGuess = Math.round(this.position / this.childSpan) % this.slides.length |
There was a problem hiding this comment.
This bestGuess is correct on regularly sized sliders.
Depending on the size differences in other childs it may only have to check one additional element.
| return Math.round( | ||
| (this.sliderSpan / (this.vertical ? this.container.scrollHeight : this.container.scrollWidth)) * this.container.children.length, | ||
| ) |
There was a problem hiding this comment.
I think this will still cause incorrect calculations for the slidesTotal now. A correct calculation here would take into account first how many items there are, and then only subtract how many items are visible when the slider is all the way at the end.
Is this value still relevant? Because on most shops we already hide the "progress bar" under sliders anyway.
…into bugfix/current-calculation
| this.$nextTick(() => { | ||
| useResizeObserver(this.slider, useThrottleFn(this.updateSpan, 150, true, true)) | ||
| setTimeout(() => this.slider.dispatchEvent(new CustomEvent('scroll'))) | ||
| this.mounted = true |
There was a problem hiding this comment.
This new order is necessary due to 176b2cd#diff-2d53f035dc6e0201915f81219d324c2479c3acd61fde0b67ffbc091205965494R222
| this.handleLoopBoundary() | ||
| index = Math.min(Math.max(index, 0), this.container?.children.length - 1) | ||
| if (this.loop) { | ||
| if (index < this.slides.length - 1) { | ||
| index += this.slides.length | ||
| } else if (index > this.slides.length * 2 + 1) { | ||
| index -= this.slides.length | ||
| } | ||
| } |
There was a problem hiding this comment.
By calling handleLoopBoundary and recalculating the index depending on wether loop is enabled. we can ensure scrolling remains smooth while resetting the scroll position.
e.g. user is on the final slide but presses the right button again while scrolling.
before it may use smooth scrolling to scroll back instead of instant, or it might not scroll back at all.
now we first handle the loop boundary, resetting instant to the first slide, then recalculate what the index should be and smoothly scroll to the next slide
| showLeft() { | ||
| return !(this.vertical ? this.arrivedState.top : this.arrivedState.left) | ||
| }, | ||
| showRight() { | ||
| return !(this.vertical ? this.arrivedState.bottom : this.arrivedState.right) | ||
| }, |
There was a problem hiding this comment.
By using useScroll we can heavily simplify show left and show right. Since it simply checks wether we've reached the end of the slider we no longer need to check wether we're looping
| this.scrollX = x | ||
| this.scrollY = y | ||
| this.isScrolling = isScrolling | ||
| this.arrivedState = arrivedState |
There was a problem hiding this comment.
All these values are now reactive, meaning we can use listeners and simple setters to set the position.
|
3 participants
This PR does the following to fix the bugs with irregularly sized sliders:
This way slides with different sizes no longer interfere with the current slide number