Ember-Swiper / ember-cli-swiper

Swiper-Wrapper for ember
https://ember-swiper.github.io/ember-cli-swiper/
MIT License
72 stars 75 forks source link

Help Creating Large Carousel With Dynamic Content #115

Closed danDanV1 closed 5 years ago

danDanV1 commented 5 years ago

Swiper slides are filtered ember data, which is always 7 records. As the user swipes through the slides, depending on the direction, we add a record to the end of the array and remove a record from the start, and vice versa for reverse.

Slides as filtered data:

{{#each filteredItems as |item|}}
        {{#swiper-slide}}
              {{image-component item=item}}
        {{/swiper-slide}}
      {{/each}}

Observer is set to watch changes to the DOM: Swiper container options/events:

      events=(hash
        observerUpdate=(action "onObserverUpdate")
        slideChangeTransitionEnd=(action "onSlideChangeTransitionEnd")
      )
      options=(hash observer=true)

Actions:

    onSlideChangeTransitionEnd() {
      this.calcCurrentItem(); //find the current slide within our model of 100+ slides for filtering
      this.filterItems(); //Slides in DOM updated here
    },

    onObserverUpdate() {
      //swiper has received new data in the dom
      let currentSlide = this.model.items.objectAt(this.currentItem);

     //out of the 7 records given to swiper, which should be the current slide?
      this.set("filteredIndex", this.filteredItems.indexOf(currentSlide));

     //set swiper to be showing the correct slide now that the slide data has changed
      set(this.swiperInstance._swiper, "activeIndex", this.filteredIndex);
      this.swiperInstance._swiper.update();
    },

If you swipe slowly, then it works as expected. You get an infinite carousel with only ever 7 slides in the DOM at any time.

However, if you swipe fast, it breaks. Swiper will advance to the next, next slide before processing the update to the DOM. It basically increments activeIndex before firing the slideChangeEnded event for the previous slide.

I've tried all manner of available events to catch/work around with no success.

Is there a better way to design this? DDUA style? How has everyone else been handling add/removal of slides?

Matt-Jensen commented 5 years ago

Thanks for raising this concern. However it's a bit difficult for me wrap my head around this problem. It sounds like the core of the issue isn't around Ember Data, or DDAU design, but when the Swiper API is triggering the observerUpdate event?

There's other concerns I have about about this bug report such as wanting to rerender DOM on every transition end event. That just sounds incredibly expensive, especially if you have 100+ items to constantly rerender! If that is in fact what's going on, I'd consider solving this serious performance issue before we conclude there's an issue with Swiper's event API.

Does that sound reasonabale?

danDanV1 commented 5 years ago

Aside from your points about performance, the issue I have presented should still exists if you update the slide list just one time (specifically removing x# slides from the beginning of the list).

I'd like to know how others are implementing add slide and remove slide? and if my implementation isn't ideal, how should it be done?transitionEnd seems like the logical place to update the slides, or perhaps the reachEnd event.

Not matter which event you choose, and if you only choose to update the lists of slides one time only using my implementation above, it seems like the issue of swiping fast before dom render is complete will still break the activeIndex. Is there a better way to do this?

Matt-Jensen commented 5 years ago

Aside from your points about performance, the issue I have presented should still exists if you update the slide list just one time (specifically removing x# slides from the beginning of the list).

Alright so we my have to break this issue into 2 separate ones. Sounds like you are reporting a bug here. Your concern is that Swiper is not emitting events you configured? Is that a fair summary?

I'd like to know how others are implementing add slide and remove slide? and if my implementation isn't ideal, how should it be done?transitionEnd seems like the logical place to update the slides, or perhaps the reachEnd event.

I'm not sure this library is a good fit for you. Is there a reason you can't using some sort of infinite scrolling library? Carousels just seem like a completely different use case IMO, but obviously I don't fully know your requirements.

Not matter which event you choose, and if you only choose to update the lists of slides one time only using my implementation above, it seems like the issue of swiping fast before dom render is complete will still break the activeIndex. Is there a better way to do this?

I'm not trying to say there's not a bug in this addon, but generally there's just so much going on in that example above (as well as code that's not included) it's impossible to say for sure. The only way I can help you debug is if you give me some code with the reproducible error. Or best of all: a failing test.

sdhull commented 5 years ago

@edeis53 could you speak to why you opted not to use the built-in loop=true (eg, {{#swiper-container loop=true}})? Seems much easier than manually modifying your array.

As for adding & removing slides, we're currently working on this very thing but I suspect that passing either updateFor=array or maybe updateFor=array.length will work.

@Matt-Jensen re: adding & removing slides, would you be open to a PR that modifies the contextual slide component to call back to forceUpdate such that update is automatically called after slides are added or removed (or possibly re-rendered)?

Matt-Jensen commented 5 years ago

@sdhull I will consider any PR that includes a test proving there's a bug in the existing code.

Matt-Jensen commented 5 years ago

Hopefully this clarifies why I'm concerned there's a footgun in this conversation:

Swiper slides can contain dynamic content, like any Ember template (unless you use loop=true which copies DOM). So making Swiper responsible for setting up and tearing down the same DOM/events for each slide, because your slides contain dynamic content, doesn't make sense and will cause performance and UX issues. Unless you actually need to change the length of the carousel, in which case updateFor=array.length sounds like a reasonable solution, to hopefully avoidable problem.

That said here's how I would solve this UX requirement of recreating a Tinder-like user experience (what I'm assuming you mean by "infinite carousel"). I would create a Swiper instance with a lot of slides (as many as would not impact perceived performance) and populate each slide with content as it becomes visible to the user as well as removing non-visible slide content. When the user reaches the end of the Slider (infinite is a theoretical term after all) I would either create a new Slider instance or use updateFor to trick the user to starting back at slide zero. However I'm sure someone can improve upon this approach, it's just what I would try to implement first.

If someone finds a bug in the code related to this thread please open a new issue.

I'm changing the title of this issue to draw discussion of how to handle large carousels with dynamic content. Bug fixes should now be discussed elsewhere.