Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breakpoints + Wraparound Not Working with Dynamic List Length #454

Closed
dinahcoe opened this issue Dec 11, 2024 · 4 comments
Closed

Breakpoints + Wraparound Not Working with Dynamic List Length #454

dinahcoe opened this issue Dec 11, 2024 · 4 comments

Comments

@dinahcoe
Copy link

dinahcoe commented Dec 11, 2024

Bug Description
I upgraded vue3-carousel in my project from v0.3.1 to v0.9.0 yesterday, and it is causing an infinite loop that crashes my computer if the number of items in the carousel is equal to the max amount of items visible.

I am using ajax calls to load carousel slide data from server side, so I had written the breakpoints as a computed property since the number of slides may be different for each user. This worked perfectly fine before I upgraded my version, but after upgrading I have found that if the number of items available to the user is 4 or less, the carousel component will cause a runtime error in an infinite loop.

I have isolated the problem, and it seems like there may be something conflicting between the breakpoints and having wrapAround turned on. I tested using static breakpoint values with no wrapAround, and it seemed to work, and I tested using wrapAround with no breakpoints and it works fine, but even if I use static breakpoints with wrapAround, I am experiencing the infinite loading.

I have confirmed that if I test the feature using an account with more than 4 items available, the Carousel component works exactly as expected. I did see this feature request in the issues already, so I'm assuming this may be a known issue, but I am already performing validation on the breakpoints to ensure that the value of itemsToShow does not exceed the number of items available and I am still experiencing this problem. I also tried loading the page with the window already resized to see if it would work if the initial width of the Carousel was smaller than the max width of the items, and it still caused the runtime issue.

To Reproduce
Steps to reproduce the behavior:

  1. Install vue3-carousel in Vue/Vite project
  2. Follow basic vue3-carousel example for setting up a Carousel component with Navigation.
  3. In the settings config for the Carousel component, include wrapAround: true and values for the breakpoints property.
  4. Load the Carousel with a data object containing a number of items that is equal to the maximum value defined for itemsToShow.
  5. Build project and deploy to development environment.

Expected behavior
The Carousel should display a number of Slides based on the width of the viewport and the Carousel should wrap around to the first Slide if it reaches the last Slide in the list. Carousel should disabled wrap around only if the total number of Slides is equal to the maximum value of itemsToShow, and should enable wrap around if the viewport is resized to a width that correlates to a breakpoint with a value for itemsToShow that is less than the total number of Slides.

Console Error
I get thousands of instances of this type of error in the console when the page tries to load:
TypeError: can't access property "vnode", n[((s + n.length) % n.length)] is undefined

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser: Firefox Developer Edition
  • Version: vue3-carousel [v0.9.0], vue [v0.9.0], vite [v6.0.3], @vitejs/plugin-vue [v5.2.1]

Additional context
Here are is a full example of my code (with any identifying specifics removed) that was working as expected prior to updated to v0.9.0.
I apologize for the long block of code, but the reason I have so many breakpoints is because the slides need to remain the same width even if the width of the carousel changes, and the values jump around because the layout of the page changes at different screen sizes.

<template>
	<div class="items">
		<div class="header">
			<h2>Title</h2>
			<select
				ref="itemsFilter"
				class="filter"
				v-on:change="filterItems"
			>
				<option value="newest">Newest</option>
				<option value="oldest">Oldest</option>
				<option value="type">Type</option>
				<option value="alpha-az">Alphabetical A-Z</option>
				<option value="alpha-za">Alphabetical Z-A</option>
			</select>
		</div>
		<div class="item-list">
			<Carousel ref="myCarousel" v-bind="settings" v-model="selectedItem" :breakpoints="breakpoints">
				<Slide v-for="slide in filteredItems" :key="slide">
					<div class="carousel__item"
					     @click="selectItem(slide - 1)"
					     @keydown.enter="selectItem(slide - 1)">
						<img
							tabindex="0"
							:class="['item',slide.name === item ? 'selected' : '']"
							:src="slide.image"
							:alt="slide.name"
						>
					</div>
				</Slide>

				<template #addons>
					<Navigation>
						<template #next>
							<div class="arrow right"/>
						</template>
						<template #prev>
							<div class="arrow left"/>
						</template>
					</Navigation>
				</template>
			</Carousel>
		</div>
	</div>
</template>

<script>
import {Carousel, Slide, Navigation} from 'vue3-carousel'
import 'vue3-carousel/dist/carousel.css'

export default {
	name: "Items",
	data: function () {
		return {
			search: '',
			// carousel settings
			currentSlide: 0,
			settings: {
				itemsToShow: 1,
				snapAlign: 'center',
				wrapAround: true,
				/*breakpoints: {
					250: {
						itemsToShow: 1.5,
						snapAlign: 'start',
					},
					300: {
						itemsToShow: 2,
						snapAlign: 'start',
					},
					350: {
						itemsToShow: 2.5,
						snapAlign: 'start',
					},
					400: {
						itemsToShow: 3,
						snapAlign: 'start',
					},
					450: {
						itemsToShow: 3.5,
						snapAlign: 'start',
					},
					500: {
						itemsToShow: 4,
						snapAlign: 'start',
					},
					551: {
						itemsToShow: 3.5,
						snapAlign: 'start',
					},
					600: {
						itemsToShow: 4,
						snapAlign: 'start',
					},
					650: {
						itemsToShow: 4.5,
						snapAlign: 'start',
					},
					675: {
						itemsToShow: 5,
						snapAlign: 'start',
					},
					700: {
						itemsToShow: 5.5,
						snapAlign: 'start',
					},
					751: {
						itemsToShow: 2.5,
						snapAlign: 'start',
					},
					824: {
						itemsToShow: 3,
						snapAlign: 'start',
					},
					924: {
						itemsToShow: 3.5,
						snapAlign: 'start',
					},
					1024: {
						itemsToShow: 4,
						snapAlign: 'start',
					}
				},*/
				gap: 6,
			}
		}
	},
	props: [
		"data",
		"item"
	],
	components: {
		Carousel,
		Slide,
		Navigation
	},
	computed: {
		selectedItem() {
			return this.currentSlide
		},
		filteredItems() {
			if (this.data) {
				let itemData = Object.values(this.data)
				if (this.search === '' || this.search.toLowerCase() === "newest") {
					return itemData
				} else if (this.search.toLowerCase() === "oldest") {
					return itemData.slice().reverse()
				} else if (this.search.toLowerCase() === "type") {
					return itemData.sort((a, b) => a.type.localeCompare(b.type))
				} else if (this.search.toLowerCase() === "alpha-az") {
					return itemData.sort((a, b) => a.name.localeCompare(b.name))
				} else if (this.search.toLowerCase() === "alpha-za") {
					return itemData.sort((a, b) => b.name.localeCompare(a.name))
				}
			} else {
				return []
			}
		},
		breakpoints() {
			const numItems = this.filteredItems && this.filteredItems.length ? this.filteredItems.length : 1;
			const getItemsToShow = function (numShown) {
				return numItems > numShown ? numShown : numItems
			}
			return {
				250: {
					itemsToShow: getItemsToShow(1.5),
					snapAlign: 'start',
				},
				300: {
					itemsToShow: getItemsToShow(2),
					snapAlign: 'start',
				},
				350: {
					itemsToShow: getItemsToShow(2.5),
					snapAlign: 'start',
				},
				400: {
					itemsToShow: getItemsToShow(3),
					snapAlign: 'start',
				},
				450: {
					itemsToShow: getItemsToShow(3.5),
					snapAlign: 'start',
				},
				500: {
					itemsToShow: getItemsToShow(4),
					snapAlign: 'start',
				},
				551: {
					itemsToShow: getItemsToShow(3.5),
					snapAlign: 'start',
				},
				600: {
					itemsToShow: getItemsToShow(4),
					snapAlign: 'start',
				},
				650: {
					itemsToShow: getItemsToShow(4.5),
					snapAlign: 'start',
				},
				675: {
					itemsToShow: getItemsToShow(5),
					snapAlign: 'start',
				},
				700: {
					itemsToShow: getItemsToShow(5.5),
					snapAlign: 'start',
				},
				751: {
					itemsToShow: getItemsToShow(2.5),
					snapAlign: 'start',
				},
				824: {
					itemsToShow: getItemsToShow(3),
					snapAlign: 'start',
				},
				924: {
					itemsToShow: getItemsToShow(3.5),
					snapAlign: 'start',
				},
				1024: {
					itemsToShow: getItemsToShow(4),
					snapAlign: 'start',
				}
			}
		}
	},
	methods: {
		selectItem(slide) {
			this.currentSlide = slide
			this.$emit('load', slide.name)
		},
		filterItems(e) {
			this.search = e.target.value
			this.$refs.myCarousel.slideTo(0)
		}
	}
}
</script>

I left in both the computed breakpoints and the hard-coded breakpoints, however please note I am experiencing this error even if I use hard-coded breakpoints and remove the ':breakpoints="breakpoints"' from the Carousel component. If I comment out either the wrapAround or the breakpoints property of the settings, it works and still loads, but with both of them together it does not work.

Before updating to v0.9.0, if the user clicked one of the items in the Carousel, it would automatically slide to that item, which worked with the wrapAround property because there were cloned slides. Although the wrapAround may not necessarily be needed if only 4 items are present (and I have classes denoting which item is selected) it would be nice if the carousel could still work in the same way even for 4 or less items because the breakpoints are still necessary if the user resizes the window.

I am open to any suggestions or corrections to my code if anyone can tell me how to make this work!

Edit:
Okay I actually had an epiphany just now that I can sort of get around it by changing my validation function to

const getItemsToShow = function(numShown) {
	return numItems > numShown ? numShown : numItems - 1
}

And this (almost) Works! I can load the page and it shows 3 out of 4 items, but if I resize the window, then it triggers the bug again... Regardless, I would still be submitting this as a bug even if the almost-solution worked perfectly because I would like to be able to show the maximum amount of viewable Slides if there is enough space to do so and not have to reduce the itemsToShow by 1 just to avoid this extreme bug.

If there is anything that can be suggested to help me solve this problem until this issue has been patched, I would greatly appreciated it as this is a live feature and I we have pending releases for it, so I need to solve it quickly and hope that it can work exactly as it did before the update!

@dinahcoe
Copy link
Author

dinahcoe commented Dec 12, 2024

I was able to get a working solution for the time being, although it is not exactly what I was hoping to achieve but it is certainly close enough and I have been wasting too much time on this so it will do for now until a better fix can be implemented!

I turned the wrapAround property into a computed property as well (not sure why it took me so long to think of that!) so that it's only set to true if the value of itemsToShow is less than the number of items in the data list.
I also implemented a fix that I found here from AndreyMyagkov, I'm not actually sure if it's even helping anything or if my new computed properties fixed it alone, but I don't want to bother with it anymore so I'm leaving it in for now!
(I also had to change some stuff about the way I was doing the slide selection on click, but that is unrelated to this bug report)

Updated Carousel component:

<Carousel ref="myCarousel" v-bind="settings" v-model="selectedItem" :wrapAround="wrapAround" :breakpoints="breakpoints">
	<Slide v-for="(slide, key) in filteredItems">
		<div class="carousel__item"
		     @click="selectItem(slide.name, key)"
		     @keydown.enter="selectItem(slide.name, key)">
			<img
				tabindex="0"
				:class="['sc-item',slide.name === item ? 'selected' : '']"
				:src="slide.image"
				:alt="slide.name"
			>
		</div>
	</Slide>

	<template #addons>
		<Navigation>
			<template #next>
				<div class="arrow right"/>
			</template>
			<template #prev>
				<div class="arrow left"/>
			</template>
		</Navigation>
	</template>
</Carousel>

Added this outside the export function (from AndreyMyagkov)

const debounce = function (fn, delay) {
  let timerId;
  return function (...args) {
    if (timerId) {
      clearTimeout(timerId);
    }
    timerId = setTimeout(() => {
      fn(...args);
      timerId = null;
    }, delay);
  };
}

Updated data:

data: function () {
	return {
		isMounted: false,
		search: '',
		// carousel settings
		currentSlide: 0,
		settings: {
			itemsToShow: 1,
			snapAlign: 'center',
			wrapAround: false,
			gap: 6,
		}
	}
}

Added this under computed properties:

wrapAround() {
	if (!this.isMounted) return
	return this.$refs.myCarousel.data.config.itemsToShow < this.filteredItems.length
}

I reverted the getItemsToShow '-1' fix from my original edit, because it was no longer needed!

And then updated and added some more stuff:

methods: {
	selectItem(name, index) {
		this.currentSlide = index
		this.$emit('load', name)
	},
	filterItems(e) {
		this.search = e.target.value
		this.$refs.myCarousel.slideTo(0)
	},
	onResize() {
		if (!this.$refs.myCarousel) {
			return
		}
		this.$refs.myCarousel.updateSlidesData()
	}
},
mounted() {
	this.isMounted = true
	window.addEventListener('resize', debounce(this.onResize, 200), {
		passive: true,
	})
	setTimeout(() => {
		this.onResize();
	}, 500)
},
beforeUnmount() {
	window.removeEventListener("resize", this.onResize)
}

Now what it does is set wrapAround to false as long as the value of itemsToShow is greater than or equal to the number of slides. I don't love that it doesn't work exactly as it did before, but it works well enough and it is fine!

I still feel that there is a bug to be fixed here, as even with the validation checking I was performing on itemsToShow, I could only get it to work as long as itemsToShow < slidesCount, it was not working when itemsToShow = slidesCount as long as wrapAround was set to true.

ismail9k added a commit that referenced this issue Dec 12, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Related to #454

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/ismail9k/vue3-carousel/issues/454?shareId=XXXX-XXXX-XXXX-XXXX).
@Tofandel
Copy link
Contributor

Tofandel commented Dec 16, 2024

Pretty sure this was fixed in #450 https://github.com/ismail9k/vue3-carousel/pull/450/files#diff-38321f8dd9c1fcb812447c55a74ea3e6fab4e37706d5b93f68735ea8d2bba14eR22-R27

Console Error
I get thousands of instances of this type of error in the console when the page tries to load:
TypeError: can't access property "vnode", n[((s + n.length) % n.length)] is undefined

As for the "infinite loop" you are referring to, you probably just have (infinite) animations on your page, triggering many slider recompute triggering the error, that was also fixed in #441

So version 0.10.0 (which seems not released yet) should work properly

@ismail9k
Copy link
Owner

@dinahcoe v0.10.0 has been released, please confirm if this issue has been resolved

@dinahcoe
Copy link
Author

@dinahcoe v0.10.0 has been released, please confirm if this issue has been resolved

IT WORKS PERFECTLY NOW THANK YOU SO MUCH!
This was a very fast response! Granted, I think you were already working on these fixes before I posted, but it is just very nice to see problems being addressed in real time, thank you so much for your hard work! ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants