mistic100 / jQCloud

jQuery plugin for drawing neat word clouds that actually look like clouds
mistic100.github.io/jQCloud
MIT License
268 stars 106 forks source link

Improvement: method 'resize' is now available publicly #15

Closed SamyCookie closed 8 years ago

SamyCookie commented 8 years ago

I need to have an access to the 'resize' method outside. So, I made it public.

mistic100 commented 8 years ago

There are some mistakes here, it works only because you are lucky.

  1. When the resize method is created with throttle, "this" does not exist, because this part of the code is not an instance, it's only the creation of the class/prototype. But it works because there is a "security" on line 480 (context || that) Anyway the resize method itself should not be throttled.
  2. Line 163 I don't even understand where "data" come from...

So I propose the change lines 162-164 into

$(window).on('resize', throttle(this.resize, 50, this));

And remove the throttling when defining "resize".

PS: watch out your indentation

SamyCookie commented 8 years ago

1) I am not very confortable with the 'throttle' method so I made the necessary changes you described 2) https://api.jquery.com/event.data/ I sometimes use it to avoid creating an external new variable.

$(foo).on('click', this, function(evt) {
  evt.data.something();
};

instead

var self = this;
$(foo).on('click', function() {
  self.something();
};

I prefer to write the first example, even if both do the same work.

The commit has been refreshed, please review.

SamyCookie commented 8 years ago

Hi, I am using this change and unfortunately I think that the strategy used for "throttle" is wrong. "throttle" behaviour works fine with autoResize on the main window, but throttling is not used when I call 'resize' by myself. I remade this commit at https://github.com/SamyCookie/jQCloud/commit/8941bdae788b336b8629263223c1b22ad9530b2c?w=1. @mistic100 What do you think of this ?

mistic100 commented 8 years ago

As I said in my previous post, I think it's a bad idea to have a method always throttled, if you need it to be throttled, do it yourself (duplicate the throttle method in your own codebase).

SamyCookie commented 8 years ago

Ok then, I'll do that. Thanks !