Selectors clean-up: color module

color.js

there is a #preview.once in a loop, take it out.
use .on everywhere (works with delegation).
the focus function unbind and rebind events, should be delegated.
lock update looks overly complicated.

Try decoupling everything with custom events in some places (updating related colors for example).

preview.js

Select elements first, then use them somewhere. Don't reselect from the ID, select the id then .find() what's needed.

A starting point just optimizing selectors and delegates basically. No refactoring whatsoever yet.

Changing issue status

Lots of this really need a rewrite and the id-selectors don't need to be scoped. I'll look into cleaning this up later this week unless someone else gets there first :)

First! :D

Only adding a slight optimisation because it seems like all the other $form.find('#id') calls seem to depend on the outcome of a .once() call. Probably able to optimize some more, but I don't have time to investigate possible repercussions right now.

+++ b/core/modules/color/color.jsundefined
@@ -33,36 +33,37 @@ Drupal.behaviors.color = {
+        var $gradient = $preview.find('#gradient-' + i);

+++ b/core/modules/color/color.jsundefined
@@ -33,36 +33,37 @@ Drupal.behaviors.color = {
+    $form.find('#edit-scheme').on('change', function () {

+++ b/core/modules/color/color.jsundefined
@@ -33,36 +33,37 @@ Drupal.behaviors.color = {
+            callback($form.find('#edit-palette-' + fieldName), colors[fieldName], false, true);

+++ b/core/modules/color/color.jsundefined
@@ -172,7 +174,7 @@ Drupal.behaviors.color = {
+      $form.find('#edit-scheme').each(function () {

+++ b/core/modules/color/color.jsundefined
@@ -198,8 +200,9 @@ Drupal.behaviors.color = {
+    $form.find('#palette input.form-text')

+++ b/core/modules/color/preview.jsundefined
@@ -8,13 +8,15 @@
+      var $palette = $form.find('#palette');

+++ b/core/modules/color/preview.jsundefined
@@ -8,13 +8,15 @@
+      $form.find('#preview').css('backgroundColor', $palette.find('input[name="palette[base]"]').val());

+++ b/core/modules/color/preview.jsundefined
@@ -8,13 +8,15 @@
+      var $text = $form.find('#text');

+++ b/core/modules/color/preview.jsundefined
@@ -40,7 +42,7 @@
+            $form.find('#gradient-' + i + ' > div').each(gradientLineColor);

Had a quick look-through. These are all selectors selecting by id, so I think the .find() is unnecessary here?

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

detach at least?

@#6: Did you check if they depend on that .once() call, cause I think they do. Perhaps they should be wrapped in a conditional that checks if $form is populated and then just runs it on the regular ID?

I don't believe $(context).find(id) should be changed to $(id). Yes, 90% context === document, but the other 10% it doesn't, which means $context.find(id).lenght could equal 0, while $(id).length most probably always return 1.

One could argue that the .once() takes care of any double behavior happening, but if that's the case this should be documented somewhere. As this is a departure from D7 js conventions. Also, I'd think that $(context).find() is a bit more flexible than having to rely on .once().

you're right, I forgot about that. $(context).find(id) is better indeed.

tag

Fair enough, but this should then be part of the color.module component (also for our exposure) since it belongs to a specific module and is not standalone core JS. Added issue references.

5: core-1751334-5.patch queued for re-testing.

Hi, added new patch, previous patch can't be applied. :)

JSHint error about indentation at:

            // Render gradient lines.
             $form.find('#gradient-' + i + ' > div').each(gradientLineColor);

Other than that, it looks good.

Re-rolled!

Patch is outdated

Rerolled patch, fixed merge conflicts.

+++ b/core/modules/color/preview.js
@@ -30,15 +29,18 @@
      * @param {number} width
      *   Width of gradient.
      */
-    callback: function (context, settings, form, farb, height, width) {
+
+    callback: function(context, settings, $form, farb, height, width) {

suppose this extra line is not needed

Updated patch, cleaned with eslint.

+++ b/core/modules/color/color.js
@@ -31,9 +31,8 @@
-      $('<div class="color-placeholder"></div>').once('color').prependTo(form);
-      var farb = $.farbtastic('.color-placeholder');
...
+      $('<div id="placeholder"></div>').once('color').prependTo($form);
+      var farb = $.farbtastic('#placeholder');

we don't do other code change in clean up

Hello guys,

droplet, I removed this strings in new patch, diff & patch attached

Not sure about scope but variable renames looks reasonable
Let's get maintainers feedback

Let's do it at ES6 or at least with a JS test covered.

Color has been removed from core, #3270899: Remove Color module from core.