Add a new Drupal.theme JavaScript function for theming the the show/hide row weight toggle and its wrapper in draggable tables

Adding patch for Table Toggle handler which is now dynamic across -

1. --- a/core/misc/tabledrag.es6.js
   +++ b/core/misc/tabledrag.es6.js
   

   We should also compile this file using the JavaScript compilation tools: https://www.drupal.org/node/2815083.

2. +++ b/core/misc/tabledrag.es6.js
   @@ -210,7 +210,7 @@
   +      const tableToogleweight = Drupal.theme('tableToogleweight');
   

   Why do we need the local variable here?

Adding patch for Table Toggle handler which is now dynamic across with both compiled and es6 file in es5 format as well - https://www.drupal.org/files/issues/2019-12-23/3084916-6-Add-a-new-drupa...

#6 Took care of requirement #1, this takes care of the remaining requirements, #2 and #3.

Any thoughts on making these just one theme function? It doesn't seem like we would necessarily need the level of flexibility to allow overriding them separately.

#11 sounds like a good idea. I was just doing a 1:1 relationship, but it is simpler with one theme function.

+++ b/core/misc/tabledrag.es6.js
@@ -1707,6 +1716,18 @@
+        `<div class="tabledrag-toggle-weight-wrapper" data-drupal-tabledrag-toggle-weight-wrapper><button type="button" class="link tabledrag-toggle-weight" data-drupal-tabledrag-toggle-weight>${text}</button></div>`,

Should we use the data-drupal-selector pattern which is getting more and more common place in core?

Need to update claro tabledrag too.

data-drupal-selector +1

There is an issue here. In case there are 2 tabledrag on the page:

Before

:

1. Table 1 processed: The button is created and added before the table element in the constructor for Table 1.
2. Table 2 processed: The button is created and added before the table element in the constructor for Table 2.
3. When columns are shown showColumns update the button text for all tables on the page
4. When columns are hidden hideColumns update the button text for all tables on the page

After the patch

:

1. Table 1 processed: initialize the column visibility of Table 1 => execute updateWeightToggle.
   1. updateWeightToggle remove all toggle buttons wrappers on the page (none),
   2. select all the tables that have been 'onced' by tabledrag (here Table 1) and recreate the html + insert before each table
   3. bind the event listener on all toggle buttons on the page (for Table 1).
2. Table 2 processed: initialize the column visibility of Table 2 => execute updateWeightToggle.
1. updateWeightToggle remove all toggle buttons wrappers on the page (Table 1 toggle),
2. select all the tables that have been 'onced' by tabledrag (here Table 1 and Table 2) and recreate the html + insert before each table
3. bind the event listener on all toggle buttons on the page (for Table 1 and Table 2).
3. When columns are shown showColumns => execute updateWeightToggle (see above)
4. When columns are hidden hideColumns => execute updateWeightToggle (see above)

I would keep the existing behavior, Changing a text on all the tables toggle at once is already borderline, if one tabledrag object modify the html of other tables it's not acceptable. What would be better is react on the columnschange event and have each object take care of updating it's toggle text.

as mention by lauriiii have added JS and generate patch

#14 not addressed

@jpatel657 when you create a patch, an interdiff should also be included https://www.drupal.org/documentation/git/interdiff.

#16 looks like it may have been a mistake as there's no compiled JS and the only difference from #12 is adding the HTML markup for a weight toggle button to the div containing tabledrag warning messages. I think it would work best if the next patch builds on#12 so it's clearer from the interdiff what feedback is being addressed.

Re #14

Need to update claro tabledrag too.

This will be addressed in Claro in #3083051: Refactor tabledrag when core issues are resolved. This issue is one of several blockers for that issue.

I would keep the existing behavior, Changing a text on all the tables toggle at once is already borderline, if one tabledrag object modify the html of other tables it's not acceptable. What would be better is react on the columnschange event and have each object take care of updating it's toggle text.

This is a good point. The issue summary mentioned "fully replace the toggle" but it only needs to be possible to change the content of the toggle to get the desired flexibility. This approaches it per-table instead of globally and eliminates the need to add a new listener on each toggle.

Also adopted the data-drupal-selector suggested in #13

Looks all good to me, if green RTBC

1. +++ b/core/misc/tabledrag.es6.js
   @@ -171,6 +171,11 @@
   +    /**
   +     * @type {jQuery}
   +     */
   +    this.$toggleWeightButton = null;
   

2. We should probably make it explicit that this property is nullable.

   +++ b/core/misc/tabledrag.es6.js
   @@ -1707,6 +1712,29 @@
   +      tableDragToggle: () =>
   ...
   +      toggleButtonContent: show =>
   

   Any thoughts on calling the toggleButtonContent inside tableDragToggle? This would make it easy to add extra CSS classes to the button or the wrapper depending on whether the row weights are visible or not. We could use something like $.replaceWith for replacing the old tabledrag button with the new one.

This takes care of #23.1. I'm less certain about #23.2, but it's possible I'm not fully understanding it. I like that the current approach does not require destroying/adding an event listener - but it's possible that isn't that big a deal or can actually be avoided.

Any thoughts on calling the toggleButtonContent inside tableDragToggle? This would make it easy to add extra CSS classes to the button or the wrapper depending on whether the row weights are visible or not. We could use something like $.replaceWith for replacing the old tabledrag button with the new one.

not a fan, this theme function is to initialize the markup not to update it whenever something changes. This can be done with the columnschange event when it's shown/hidden. Using replaceWith for an element that is by default the same in the prospect of someone using it differently in contrib feels wrong.

Unless there is a compelling argument that doing replaceWith allow something that is not possible with this I'd stick to the patch in it's current form.

I don't feel too strongly about #23.2 so it seems fine to go with the current approach given that it's preferred by @bnjmnm and @nod_.

Let's still add a change record to describe the new theme functions.

Added a CR https://www.drupal.org/node/3160782

Thank you for creating the CR! Looks great!

Committed 56aa632 and pushed to 9.1.x. Thanks!