Brackets core has 8 instances of $.click()

[le717]

I have been watching some of the code review processes, and I've noticed anytime $.click() is used, there is a request to change it to $.trigger("click"). I checked the coding conventions for mention of this, but it seems to be a convention by precedent (or necessity) rather than by rule. To that end, I looked up all instances of $.click() in Brackets core (as of 7af961c), as can be seen in the image below.

(Third-party code with instances have been collapsed.)

To conform to the aforementioned convention, shouldn't these be converted to $.trigger("click")?

[TomMalbran]

@le717 Notice that $.click() is the same as $.trigger("click") and not to $.on("click"). $.trigger("click") triggers the event, while $.on("click", handler) attaches an even listener. Maybe we could replace all the instances of $.click() with $.trigger("click"), but not with $.on("click").

[le717]

@TomMalbran If I could only think straight at times.... Yes, that is what I meant, $.trigger("click"), not $.on("click"). OP fixed.

[TomMalbran]

Oh. Then yes, it is much clear to see $.trigger("click") than $.click().

[le717]

http://api.jquery.com/click/

This method is a shortcut for .on( "click", handler )..., and ``trigger( "click" )`...

Suddenly I'm confused which one to use where.

[TomMalbran]

@le717 Yes, that is kind of the issue with most of the methods in jQuery, too much method overloading. .click(handler) = .on("click", handler) and .click() = .trigger("click").

[le717]

@TomMalbran Since none of these have handlers attached to them, they translate to .trigger("click"), then? If you'd like, I'll submit a quick PR to convert them once I get the proper translation down.

[TomMalbran]

@le717 Yes, that sounds right.

[le717]

OK, wow. I forgot I had filters activated. Turns out .click() is used extensively in the tests, 86 times in InstallExtensionDialog-test alone. (I also had a dist folder created as shown in the screen shot above. I deleted it.)

I knocked out a few before I took that image, but that is a fairly accurate usage count. Sure you still want me to change them in the tests? I can still do it.

[le717]

BTW, initial replacement pass is at #8467.

[le717]

@TomMalbran All instances converted and all effected tests pass. Awaiting your response to know if I should push them or not. :)

[TomMalbran]

@le717 I actually don't know what the team wants. So maybe someone in the team can give you the ok.