jdan / dynamo.js

Cycles through bits of text in place
http://jdan.github.io/dynamo.js/
MIT License
626 stars 65 forks source link

Fix for issue#18 #19

Closed ganesshkumar closed 10 years ago

ganesshkumar commented 10 years ago

Using eval to convert the obtained string into function, so that it can be executed during the callback

ganesshkumar commented 10 years ago

That makes sense but in test.html, bump(2) will be passed as a string and it will be converted into a function like

function anonymous() {
    bump(2)
}

And on executing bump(2), it returns a function that can trigger the next digit. But this function that is returned by bump(2) is not executed, so the next digit is not triggered.

We can do this at test.html to solve this,

bump = function(id) {
  return function() {
    $('#digit' + id).dynamo_trigger();
  }()
}

or

bump = function(id) {
    $('#digit' + id).dynamo_trigger();
}

where we are executing the function and returning the statement to the callback. I am not sure is this the perfect way to do it. Let me know what do you think.

jdan commented 10 years ago

Hm, you're right. My bump seems to be poorly written, and should instead read:

bump = function(id) {
    $('#digit' + id).dynamo_trigger();
}

I'm going to contradict what I said earlier and say that we should keep line 93 the same, and instead change line 33 to

var callback = options.callback || new Function(v$.data('callback')) || function () {};

So basically just switching eval to new Function(...). How does that sound? Thanks for being patient with this.

ganesshkumar commented 10 years ago

That sounds perfect :+1:

jdan commented 10 years ago

Awesome, feel free to modify your PR and I'll make sure this gets merged. Just ping me, since pushes don't send notifications.

jdan commented 10 years ago

Fix landed in 0f710b9fbcb48d55a7017fadc4f4176ebd2279eb. Nice work!