peduarte / wallop

:no_entry: currently unmaintained :no_entry: A minimal JS library for showing & hiding things
1.1k stars 79 forks source link

Uncaught (in promise) Error #75

Closed NetOpWibby closed 5 years ago

NetOpWibby commented 8 years ago

The error is An instance of Wallop with this selector already exists. I am using Wallop in my FeathersJS app and I am dynamically generating my UI with JavaScript. I instantiate Wallop in my render code and it everything works well. When I leave the tab and go back to it, I see this error and my slides are nowhere to be found.

I'm just referencing the wallop.min.js file. Should I be using npm instead (if so, how do I use it in the frontend)?

EDIT: Adding the Wallop-item--current class to the first item in my carousel makes it at least look presentable when the Wallop instance breaks...which gives me an idea, brb.

Alright, turns out that if I just comment out

for (var i = 0; i < selectorPool.length; i++) {
  if (selectorPool[i] === selector) {
    throw new Error('An instance of Wallop with this selector already exists.');
  }
}

in the unminified version of Wallop and use that, I don't run into any issues. I'm running with that.

honzabilek4 commented 8 years ago

Hi, I'm not familiar with using FeatherJS but the idea is to instantiate the wallop object only once when page loads. Then, if the object already exists I'd only call my new method to refresh it's inner state on render. See the pending pull request #70.

NetOpWibby commented 8 years ago

That PR has been pending for almost a month. If I were to take the code inside and apply it to my code, would it work?

honzabilek4 commented 8 years ago

Yes, It should. It's just the reset method to be added.

NetOpWibby commented 8 years ago

Finally had the time to try out your fixes. I copied and pasted the minified code and overwrote my file and the issue wasn't fixed completely. There was a slight glitch effect and I'm getting those console errors again. However, the first image of my carousel didn't disappear.

honzabilek4 commented 8 years ago

Do you have some code samples or a project repository somewhere? This is too abstract to solve.

NetOpWibby commented 8 years ago

My project is closed-source for now, but here's the code for the entire page

  renderProducts();

  $(window).on("resize", function () {
    renderProducts();
  });

  function renderProducts() {
    productService.find(function (error, product) {
      var html = "";

      for (productItem of product) {
        if (productItem.Status === "Published") {
          html +=
            "<li id='" + productItem._id + "' class='home__product Wallop-item' style='background-image: url(/" + productItem.Images[0] + ");'>" +
              "<a href='" + window.location.protocol + "//" + window.location.host + "/shop/" + productItem.Slug + "'>" +
                "<h3 class='home__product__name'><span>" + productItem.Name + "</span></h3>" +
              "</a>" +
            "</li>";
        }
      }

      if (product.length > 0) {
        $(".home__products ul").html(html);
        $(".home__products ul li:first-of-type").addClass("Wallop-item--current");

        initWallop();
      }
    });
  }

  function initWallop() {
    var
      wallopEl = document.querySelector(".Wallop"),
      wallop = new Wallop(wallopEl),
      paginationDots = Array.prototype.slice.call(document.querySelectorAll(".Wallop-dot"));

    //
    //  P A G I N A T I O N
    //

    // Click dot
    paginationDots.forEach(function (dotEl, index) {
      dotEl.addEventListener("click", function () {
        wallop.goTo(index);
      });
    });

    // Update classes on change
    wallop.on("change", function (event) {
      removeClass(document.querySelector(".Wallop-dot--current"), "Wallop-dot--current");
      addClass(paginationDots[event.detail.currentItemIndex], "Wallop-dot--current");
    });

    // Helpers
    function addClass(element, className) {
      if (!element) {
        return;
      }

      element.className = element.className.replace(/\s+$/gi, "") + " " + className;
    }

    function removeClass(element, className) {
      if (!element) {
        return;
      }

      element.className = element.className.replace(className, "");
    }

    //
    //  A U T O P L A Y
    //

    autoplay(4000);

    function autoplay(interval) {
      var lastTime = 0;

      function frame(timestamp) {
        var update = timestamp - lastTime >= interval;

        if (update) {
          wallop.next();
          lastTime = timestamp;
        }

        requestAnimationFrame(frame);
      }

      requestAnimationFrame(frame);
    };
  }
peduarte commented 8 years ago

Hey guys, here's the latest release 2.4.0 https://github.com/peduarte/wallop/releases/tag/v2.4.0

Sorry it took so long, hope this will help you.

peduarte commented 8 years ago

Pushed 2.4.1 a small patch updating docs...

NetOpWibby commented 8 years ago

Thanks @peduarte! I hope it fixes my issue.

Wifsimster commented 8 years ago

Hi, thanks for this awesome plugin. However, we have a similar problem. We have a component which creates a new instance each time it's refreshed and so throw a new error.

@peduarte, you said

if the object already exists I'd only call my new method to refresh it's inner state on render

But if the selector is already in selectorPool, you directly throw a new error, see above :

if (selectorPool[i] === selector) {
        throw new Error('An instance of Wallop with this selector already exists.');
      }

I think it would be more handy returning the instance directly without any error.

If you have a better solution, i'm listening :)

peduarte commented 8 years ago

Hi @Wifsimster, when you say a component are you referring to a React component? It was @honzabilek4 who said that maybe he has a better solution?

I may consider introducing a destroy method so when you need to unmount your component you can clean up the bound events.

Also, I'm seriously thinking about a cheeky re-write as I'm not entirely happy with how the code is structured... just need some time!

honzabilek4 commented 7 years ago

Hi, I think @Wifsimster is right. It would be more handy to return directly the wallop instance. I cannot see any shortcomings. This would also solve #62. I can create the PR If you agree. Btw. @peduarte If you're thinking about a total overhaul, I can possibly help you.

peduarte commented 7 years ago

@honzabilek4 Right now I don't have time – not even to review unfortunately. But let's try and make this happen at some point in the near future. Your help will definitely be appreciated.

honzabilek4 commented 7 years ago

@peduarte okay, let me know then. I'm definitely in. 🙂