I noticed there was a todo for this when digging around modernizr.admin.inc. "TODO: $list .= $test['desc'];"

It seemed like a proper definition list, but a table may make more sense if more info is added in the future. Also, having this page's callback return a render array instead of a string would make it more flexible in the future.

I'll attach a patch in a bit.

CommentFileSizeAuthor
#4 modernizr-add-descriptions-1478038-4.patch4.8 KBpixelwhip
#1 modernizr-add-descriptions-1478038-1.patch5.4 KBpixelwhip

Comments

pixelwhip’s picture

pixelwhip commented
StatusFileSize
new modernizr-add-descriptions-1478038-1.patch5.4 KB

I went ahead and converted the page to a render array. I also made use of the .button style on the download link and moved it to the top of the page as it seems to be the focus. I fluffed the description a bit too, to hopefully make its purpose a bit more clear.

As this was written previously, I believe there would have been instances in which the test cases were not properly grouped if they weren't listed by module in the '$test' variable. This is probably an edge case, but this patch should avoid those issues.

The definition list could use some styling love in the future, but it at least has some semantic structure for now.

pixelwhip’s picture

pixelwhip commented
Status: Active » Needs review
rupl’s picture

rupl
he/him
Primary language English
commented
Status: Needs review » Needs work

Hey pixelwhip, I think your suggestions are great, however, the patch doesn't apply to the latest dev branch. 

+++ b/modernizr.admin.incundefined
+++ b/modernizr.admin.incundefined
+       // Create a definition pair for this test.
+       $elements['tests']['list'][$module]['tests'][] = array(
 -        'label' => array('#markup' => '<dt>' . $test['test'] . '</dt>'),
 -        'description' => array('#markup' => '<dd>' . $test['desc'] . '<dd>'),
++        'label' => array(
++          '#markup' => $test['test'],
++          '#prefix' => '<dt>',
++          '#suffix' => '</dt>',
++        ),
++        'description' => array(
++          '#markup' => $test['desc'],
++          '#prefix' => '<dd>',
++          '#suffix' => '</dd>',
++        ),
+       );
+ ¶
 -      $link .= $test['test'] .'-';
++      // @todo Check to see if this test has already been added by another module.
++      $link .= $test['test'] . '-';

I see extra plus and minus signs here. Could this be the issue?

+++ b/modernizr.admin.incundefined
+++ b/modernizr.admin.incundefined
+   // Create the tests description render element.
+   $elements['tests']['description'] = array(
+     '#theme' => 'html_tag',
 -    '#value' => t('Your other Drupal modues and themes have registered the following Modernizr tests:'),
++    '#value' => t('Your installed Drupal modules and themes have registered the following Modernizr tests:'),
+     '#tag' => 'p',

A few more here.. This seems to be the same typo you fixed in #1476980: Typo on Modernizr Rebuild page, perhaps you need to get the latest dev before patching?

pixelwhip’s picture

pixelwhip commented
Status: Needs work » Needs review
StatusFileSize
new modernizr-add-descriptions-1478038-4.patch4.8 KB

This patch should be much cleaner.

rupl’s picture

rupl
he/him
Primary language English
commented
Status: Needs review » Fixed

Committed to 7.x-3.x dev. Thanks!

Status: Fixed » Closed (fixed)

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