Selectors clean-up: user module

performance sensitive script, see if it can be made better.

Here's an initial cleanup patch

Had a typo on keyup.

+++ b/core/modules/user/user.permissions.jsundefined
@@ -35,16 +35,15 @@ Drupal.behaviors.permissions = {
+      $table.find('input[type=checkbox]')
+        .not('.rid-anonymous, .rid-authenticated')
+        .addClass('real-checkbox').each(function () {
+          $dummy.clone().insertAfter(this);

This could probably be further simplified into:

      $table.find('input[type=checkbox]')
        .not('.rid-anonymous, .rid-authenticated')
        .addClass('real-checkbox')
        .after($dummy);

----

+++ b/core/modules/user/user.permissions.jsundefined
@@ -35,16 +35,15 @@ Drupal.behaviors.permissions = {
+      $table.find('input[type=checkbox].rid-authenticated').each(self.toggle);

This is probably the most intensive operation but I dont really see a way to optimize it.

I changed the .each into a simple .after()

user.js:

Can you make the passwordCheck, passwordCheckMatch functions into named functions?

    var $checkbox = $('#edit-instance-settings-user-register-form');

    if ($checkbox.length) {
      $('#edit-instance-required').once('user-register-form-checkbox')

I'd rather have that as:

    var $checkbox = $('#edit-instance-settings-user-register-form');
    var $instance =  $('#edit-instance-required').once('user-register-form-checkbox');

    if ($checkbox.length && $instance.length) {

user.permissions.js:

can you separate the .once() call and the function in there? there is no need for this closure. Have a look at other core scripts to see what I mean (yay! can finally say that with some confidence :).

Beside that it's all good :) thanks a lot!

Nice, thanks.

This also saved us from a global pollution, didn't think about passwordCheckMatch() populating this with a variable and then being called as a global function. Jshint didn't catch it as a function expression either, quite odd.

A good reason for these new conventions :)

So my solution is just to remove both possible classes before adding a new one, without checking if it exists or not. If you prefer using a closure to memorize, I'll revise it.

tag

+++ b/core/modules/user/user.jsundefined
@@ -9,85 +9,89 @@
+    var $innerWrapper = $passwordInput.parent();

Give a better name ? eg $passwordInputParent.

+++ b/core/modules/user/user.jsundefined
@@ -9,85 +9,89 @@
+    function passwordCheck () {
...
+    function passwordCheckMatch () {

no space "function()"

+++ b/core/modules/user/user.jsundefined
@@ -9,85 +9,89 @@
+    $passwordInput.on('keyup focus blur', passwordCheck);
+    $confirmInput.on('keyup blur', passwordCheckMatch);

add event namespaces ? should it bind "change" event ? and "input" event for IME

+++ b/core/modules/user/user.jsundefined
@@ -99,15 +103,15 @@ Drupal.behaviors.password = {
+  var $usernameBox = $('input.username');

can we call it $usernameInput

I will update it when #1684880: JSHint user get commited

Too many changes, won't attach an interdiff.

#13: user.d8.patch queued for re-testing.

Stray semi-colon in user.persmissions.js

the $dummy change looks good, you're right, no need to clone it.

The selector can even be transformed to be

  $table.find('input[type="checkbox"]:not(.rid-anonymous, .rid-authenticated)')

works the same for me.

Changes to user.js looks good :)

I found it in user.js

diff -u b/core/modules/user/user.js b/core/modules/user/user.js
--- b/core/modules/user/user.js
+++ b/core/modules/user/user.js
@@ -84,7 +84,7 @@
       // Monitor input events.
       $passwordInput.on('input', passwordCheck);
       $confirmInput.on('input', passwordCheck);
-    };
+    }
   }
 };

2. the selectors transform I leave as it, no diff.

Nothing is perfect, Move On!.

Try again, it applied locally

Re-rolled!

Seeing a few ESLint errors:

core/modules/user/user.js
   58:16  error  "$passwordSuggestions" used outside of binding context  block-scoped-var
   59:14  error  "$passwordSuggestions" used outside of binding context  block-scoped-var
   63:12  error  "$passwordSuggestions" used outside of binding context  block-scoped-var
   78:31  error  There should be no space after '{'                      space-in-brackets
   78:55  error  There should be no space before '}'                     space-in-brackets
   81:31  error  There should be no space after '{'                      space-in-brackets
   81:54  error  There should be no space before '}'                     space-in-brackets
   98:23  error  indicatorClass is defined but never used                no-unused-vars
  185:22  error  "indicatorColor" used outside of binding context        block-scoped-var
  185:22  error  "indicatorColor" is not defined                         no-undef

core/modules/user/user.permissions.js
  12:0  error  Keyword "if" must be followed by whitespace  space-after-keywords
  12:6  error  Expected indentation of 6 characters         indent

I applied #24 corrections.
No clue for the indicatorClass warning

Need reroll :/

Looks good though.

indicatorColor is not used, why do we need that one, is anything else using it?

eslint error on the user.permission.js file.

I have problems with eslint, but here's what I suggest..

Looks good to me, eslint is happy.

Password is working as expected. Permission page too.

I've reviewed this and manually tested - all looks okay. Not the easiest issue to review as the changes seem to be from multiple scopes - internal clean ups and new js specific classes. Ideally this would be done in separate issues.

Given that we are preceding with other issues that add the js- I'm going to commit this patch as part of that effort. However, in future it would be great if the scope of the these patches could be reduced to just doing the necessary to do that. Committed 74363f6 and pushed to 8.0.x. Thanks!