Selectors clean-up: states.js

All right, haven't seen anything obvious but I want to keep it around some more.

I'll take it out of the review list for now.

BONUS: Investigate if adding basic events and detach makes sense, as described in #1763812: [META] Provide complete attach/detach with basic events

Furthermore:

$(document).bind('state:disabled', function(e) {
$(document).bind('state:required', function(e) {
$(document).bind('state:visible', function(e) {
$(document).bind('state:checked', function(e) {
$(document).bind('state:collapsed', function(e) {

Shouldn't all those use the dot as a namespace separator, or does the ":" have some special function I'm missing? Might have something to with the comment below, but I'm actually unfamiliar with the state system.

/**
 * Global state change handlers. These are bound to "document" to cover all
 * elements whose state changes. Events sent to elements within the page
 * bubble up to these handlers. We use this system so that themes and modules
 * can override these state change handlers for particular parts of a page.
 */

Looks a mighty lot like event namespacing in Prototype, should probably be reversed and changed to a dot like .on('disabled.state', fn) so themes and modules can easily unbind them all in one go.

tag

I've attached a patch below, which makes a couple minor changes to the code:

• Assigns all the $(document) calls to a $document variable to reduce recalling jQuery over and over
• Refactors a selector use to a variable since it's used in both parts of the if statement.

I don't particularly think these changes are necessary by any means, but may offer some (very minor) performance and maintainability changes to this code.

Can you replace all .bind, .click, etc with .on('event') as well? We're going for consistency :)

On it! :)

Also changed up the event binding to use .on(). Used $(document).on() which may need to change, so let me know.

The change line 121 shouldn't be there, it'll mess up the e.target value in the listeners and break everything :p

And about listeners, last nitpick.

$document.on('state:xx', function () {});
$document.on('state:yy', function () {});

can be written as

$document.on({
  'state:xx': function () {},
  'state:yy': function () {}
});

I usually don't like much of the jQueryism but you gotta admit they made a really nice event API :)

After that I'd RTBC it.

Sounds great! Here's an updated patch with:

• Reverted change at 121
• Changed the .on mentioned to use the event-map approach.

Rerolled this abandoned patch and now works :)

Sorry, JSHint complained about a comma and I have noticed now. Rerolled the patch again :(

The patch needs to be re-rolled once again.

I've taken a stab at the re-roll. I did ignore one change from the last patch as that seems to have changed.

The patch had:

-    this.element.bind(event, $.proxy(function (e) {
+    $(document).on(event, this.element, $.proxy(function (e) {

The file is now using:

this.element.on(event, $.proxy(function (e) {

Let see what says the testbot.

Thanks @rballou!

Unless it's something avoid Drupal code standard, I think the patch is fine. Even, I will close it as won't fix. The performance gain is sooo little.

#9 suggestion is too personal preference :)
https://drupal.org/comment/8183337#comment-8183337

Re-rolled!

Patch is outdated

Reroll of #20

Thanks.

Committed/pushed to 8.1.x, thanks!

Since this is clean-up only and not a functional bug fix, not committing to 8.0.x.