Problem

  1. The JavaScript of the Simpletest UI relies on all test information to be duplicated into JavaScript settings, which ~duplicates the total HTML page size.

Proposed solution

  1. Remove those JS settings entirely and properly use jQuery instead.
CommentFileSizeAuthor
#10 interdiff.txt795 bytessun
#10 test.js_.10.patch10.15 KBsun
#3 interdiff.txt1.03 KBsun
#3 test.js_.3.patch10.07 KBsun
#1 test.js_.1.patch10.2 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
test.js_.1.patch10.2 KB

Attached patch re-implements all the existing behaviors without unnecessarily relying on those JS settings.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/simpletest/simpletest.js
@@ -3,98 +3,62 @@
+      $(context).find('.simpletest-group').once('simpletest-group-collapse', function () {

    We should use a data attribute to select this. 

  2. +++ b/core/modules/simpletest/simpletest.js
@@ -3,98 +3,62 @@
+            if ($group.hasClass('expanded')) {
+              $group.removeClass('expanded');
+              $tests.addClass('js-hide');
+              $image.html(drupalSettings.simpleTest.images[0]);

    This should be using toggleClass() instead of the if / else. Very glad to see the animation go.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.js_.3.patch10.07 KB
interdiff.txt1.03 KB

Changed to use toggleClass() — although in this particular case, toggleClass() makes the logic significantly harder to understand.

The .simpletest-group class on the table row is also used for styling purposes, so I do not think it makes sense to change that into a data attribute.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

We want to decouple JS from styling so not using a class let themers do whatever they want. Probably not needed here but it's to keep consistent (this is agreed with CSS people and getting rolled out... eventually).

If you name the variable expanded, toggleClass makes a bit more sense (to me anyway).

+expand really not a fan of that but whatever, simpletest has never been the best script around and if people take it as an example they're pretty doomed :p

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Yeah, I can see that using a data attribute would make sense elsewhere... but in this case, the HTML markup (table) + JS + CSS is pretty tightly coupled... and very unlikely to be customized or overridden by anyone ;-) — so let's keep it simple? :)

First, I actually had expanded (or rather isExpanded), but that made the toggleClass() calls even more confusing, since all of them would be negated:

$tests.toggleClass('js-hide', isExpanded);

Therefore, I went with the non-negated expand (which is meant to read as "do expand").

Anyway... the primary objective here is to get rid of the completely duplicated test list in JS settings. Anything else? :-)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Reviewed & tested by the community

I'm happy enough the animation is gotten rid of.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestTestForm.php
@@ -144,7 +140,6 @@ public function buildForm(array $form, array &$form_state) {
         $test_id = drupal_clean_id_identifier($class);
         $test_checkbox_id = 'edit-tests-' . $test_id;
-        $current_js['testNames'][] = $test_checkbox_id;

$test_id and $test_checkbox_id are no longer needed.

+++ b/core/modules/simpletest/simpletest.js
@@ -3,98 +3,56 @@
+          $testCheckboxes.each(function () {
+            if (!$(this).prop('checked')) {
+              allChecked = false;
+              return false;
+            }
+          });

This return is unnecessary.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

The retun stops the iteration, which is good because there are a lot of checkboxes. Getting out earlier is what we'd want.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

#8 sure - but is the FALSE necessary?

EDIT: Answering my own question - https://forum.jquery.com/topic/breaking-the-each-loop - indeed it is.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community
FileSize
test.js_.10.patch10.15 KB
interdiff.txt795 bytes

Removed obsolete variables from SimpletestTestForm.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Ok, looks like Alex's feedback was addressed.

Committed and pushed to 8.x. Thanks!

  • Commit 0935e4c on 8.x by webchick: 
    Issue #2215049 by sun: Stop duplicating entire test list table into...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.