Remove tag fix #82

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

Fixed a bug in removeTag which was throwing an error when tags were being deleted. Also stubbed in easing effect, will possibly work on implementing this if it seems useful to others (perhaps overkill for this widget?).

benjaminapetersen added some commits Apr 11, 2012

fixed the removeTag() method so that it will no longer throw errors w…
…hen a tag is deleted. The widget will now let users delete a tag and re-enter it again, since it is truly removed. Also stubbed in the option to allow easing functionality in animation methods, though did not work this out extensively. Made a few comments where variables could use the var keyword to eliminate globals.

+1 -- fixed my problem, thanks

rock on.

@@ -353,8 +358,13 @@
},
removeTag: function(tag, animate) {
+
+ // var keyword should be used to avoid creating a global.
@aehlke

aehlke Jun 27, 2012

Owner

Good catch, since this is an optional parameter, animate may be undefined.

@ethanresnick

ethanresnick Aug 6, 2012

Fwiw, a var isn't actually needed here either. Even if animate isn't passed in, it's still bound as a local variable in the function (or, technically, as a property on the function's environment context) because it was listed as an argument; it's just bound with the value undefined. See http://ecma262-5.com/ELS5_HTML.htm#Section_10.5

@aehlke

aehlke Nov 22, 2012

Owner

You're right of course, thanks!

animate = animate || this.options.animate;
+ // var keyword to avoid global!
+ // var tag = $(tag);
@aehlke

aehlke Jun 27, 2012

Owner

However this should never be undefined unless there is a programmer error, since tag is required, and so this will always bind to the tag parameter passed to removeTag. So no need for var here.

@benjaminapetersen

benjaminapetersen Jun 27, 2012

Good call Alex. You are right.

@benjaminapetersen

benjaminapetersen Jul 18, 2012

Alex, would you like me to do anything else to this? I can cut my comments out & clean up the items you mentioned so it is a clean commit.

@aehlke

aehlke Jul 18, 2012

Owner

Hey Benjamin, sorry - meant to do this but got busy. If you want to submit a pull request with the changes discussed ill gladly pull it, thanks.

Sent from my iPhone

On Jul 18, 2012, at 11:24 AM, "Benjamin A. Petersen"reply@reply.github.com wrote:

        animate = animate || this.options.animate;
  •        // var keyword to avoid global!
    
  •        // var tag = $(tag);  
    

Alex, would you like me to do anything else to this? I can cut my comments out & clean up the items you mentioned so it is a clean commit.

Benjamin A. Petersen
ben@benjaminapetersen.me (mailto:ben@benjaminapetersen.me)
linkedin.com/in/benjaminapetersen (http://www.linkedin.com/in/benjaminapetersen)
facebook.com/benjamin.a.petersen (http://www.facebook.com/benjamin.a.petersen)
twitter.com/#!/bapetersen (https://twitter.com/#!/bapetersen)
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Wednesday, June 27, 2012 at 9:50 AM, Ben Petersen wrote:

Good call Alex. You are right.

Benjamin A. Petersen
ben@benjaminapetersen.me (mailto:ben@benjaminapetersen.me)
linkedin.com/in/benjaminapetersen (http://www.linkedin.com/in/benjaminapetersen)
facebook.com/benjamin.a.petersen (http://www.facebook.com/benjamin.a.petersen)
twitter.com/#!/bapetersen (https://twitter.com/#!/bapetersen)
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Wednesday, June 27, 2012 at 11:49 AM, Alex Ehlke wrote:

animate = animate || this.options.animate;

  • // var keyword to avoid global!
  • // var tag = $(tag);

However this should never be undefined unless there is a programmer error, since tag is required, and so this will always bind to the tag parameter passed to removeTag. So no need for var here.


Reply to this email directly or view it on GitHub:
https://github.com/aehlke/tag-it/pull/82/files#r1061040


Reply to this email directly or view it on GitHub:
https://github.com/aehlke/tag-it/pull/82/files#r1188738

@benjaminapetersen

benjaminapetersen Jul 19, 2012

No problem! I'll clean it up and issue one.

Benjamin A. Petersen
ben@benjaminapetersen.me (mailto:ben@benjaminapetersen.me)
linkedin.com/in/benjaminapetersen (http://www.linkedin.com/in/benjaminapetersen)
facebook.com/benjamin.a.petersen (http://www.facebook.com/benjamin.a.petersen)
twitter.com/#!/bapetersen (https://twitter.com/#!/bapetersen)
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Wednesday, July 18, 2012 at 10:24 AM, Alex Ehlke wrote:

animate = animate || this.options.animate;

  • // var keyword to avoid global!
  • // var tag = $(tag);

Hey Benjamin, sorry - meant to do this but got busy. If you want to submit a pull request with the changes discussed ill gladly pull it, thanks.

Sent from my iPhone

On Jul 18, 2012, at 11:24 AM, "Benjamin A. Petersen"<reply@reply.github.com (mailto:reply@reply.github.com)> wrote:

animate = animate || this.options.animate;

  • // var keyword to avoid global!
  • // var tag = $(tag);

Alex, would you like me to do anything else to this? I can cut my comments out & clean up the items you mentioned so it is a clean commit.

Benjamin A. Petersen
ben@benjaminapetersen.me (mailto:ben@benjaminapetersen.me)
linkedin.com/in/benjaminapetersen (http://www.linkedin.com/in/benjaminapetersen)
facebook.com/benjamin.a.petersen (http://www.facebook.com/benjamin.a.petersen)
twitter.com/#!/bapetersen (https://twitter.com/#!/bapetersen)
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Wednesday, June 27, 2012 at 9:50 AM, Ben Petersen wrote:

Good call Alex. You are right.

Benjamin A. Petersen
ben@benjaminapetersen.me (mailto:ben@benjaminapetersen.me)
linkedin.com/in/benjaminapetersen (http://www.linkedin.com/in/benjaminapetersen)
facebook.com/benjamin.a.petersen (http://www.facebook.com/benjamin.a.petersen)
twitter.com/#!/bapetersen (https://twitter.com/#!/bapetersen)
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Wednesday, June 27, 2012 at 11:49 AM, Alex Ehlke wrote:

animate = animate || this.options.animate;

  • // var keyword to avoid global!
  • // var tag = $(tag);

However this should never be undefined unless there is a programmer error, since tag is required, and so this will always bind to the tag parameter passed to removeTag. So no need for var here.


Reply to this email directly or view it on GitHub:
https://github.com/aehlke/tag-it/pull/82/files#r1061040


Reply to this email directly or view it on GitHub:
https://github.com/aehlke/tag-it/pull/82/files#r1188738


Reply to this email directly or view it on GitHub:
https://github.com/aehlke/tag-it/pull/82/files#r1189922

Owner

aehlke commented Nov 23, 2012

@benjaminapetersen @darrinholst I'm going through the pull request backlog and I don't understand the bug here, nor how your change could have fixed something. Can you tell me how to reproduce the error? What errors were being thrown?

I tried creating a jsfiddle to show the problem, but was unable to reproduce it. It is still happening for me on my real project though so I don't know what the difference is.

The error is...

error

and the console output of that was...

console

Ah, I figured out what the difference is. I don't include all of jquery-ui. Once I started including the blind effect it started working. Must have been getting into some jquery code that it shouldn't have been in.

Owner

aehlke commented Nov 23, 2012

Ah ok. Maybe you were using a custom built version of jQuery UI. Closing this issue then, thanks for the details.

@aehlke aehlke closed this Nov 23, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment