This is a follow up issue of #1979028: Convert help_main() to a Controller.

Updated: Comment #37

Problem/Motivation

Main help page should be themable (improved rendering).

Proposed resolution

Create a Twig template for the themeable output.

Remaining tasks

Further patch reviews
CSS needs to be adjusted for the new markup
Manual testing

Steps to reproduce

Navigate to /admin/help.

User interface changes

TBD

API changes

A new preprocess function will be added if proposed patches are applied:

function help_preprocess_block(&$variables) {
    $variables['attributes']['role'] = 'complementary';
    }
}

TBD

Original report by plopesc

This is a follow up issue of #1979028: Convert help_main() to a Controller. Main help page should be generated as a themable function.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Status: Active » Needs review
FileSize
3.72 KB

Hello

Attaching first approach to convert help page rendering to a themable function.

Regards.

babruix’s picture

Assigned: Unassigned » babruix
Status: Needs review » Needs work
Issue tags: +Needs reroll
babruix’s picture

Assigned: babruix » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.99 KB

Status: Needs review » Needs work

The last submitted patch, theme_help-1986164-3.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.52 KB

Status: Needs review » Needs work

The last submitted patch, theme_help-1986164-5.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
4.03 KB

Ok, re-rolled manually instead of using automatic rebase/merge.

disasm’s picture

star-szr’s picture

Issue tags: +Twig

We're trying not to add any new theme functions in D8 unless performance is critical, but that is usually only the case for really small, atomic templates like field templates.

#1757550: [Meta] Convert core theme functions to Twig templates

So I suggest if we are making themeable output here, this should be a template instead of a theme function.

Nigel_S’s picture

Assigned: Unassigned » Nigel_S
Issue summary: View changes
chanderbhushan’s picture

7: theme_help-1986164-7.patch queued for re-testing.

royalty’s picture

Issue summary: View changes
royalty’s picture

Issue summary: View changes
Nigel_S’s picture

Currently working on conversion of last working patch to Twig template per #9

star-szr’s picture

Assigned: Nigel_S » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs issue summary update

Unassigning since it's been a while, @Nigel_S if you're cooking something up feel free to reassign :)

Thanks @royalty for the issue summary update.

I don't think at this point this patch needs tests, since it would be similar to a Twig conversion (#1757550: [Meta] Convert core theme functions to Twig templates). Manual testing is the way to go for these.

jhodgdon’s picture

I just found another (very old) issue that duplicates this one, which had a D7 patch:
#232466: theme help topics

babruix’s picture

FileSize
4.67 KB
babruix’s picture

Status: Needs work » Needs review

My first Twig template :)

Status: Needs review » Needs work

The last submitted patch, 17: theme_help-1986164-17.patch, failed testing.

star-szr’s picture

Nice! Thanks @babruix. I suspect the test is whitespace sensitive so that may need some tweaking.

  1. +++ b/core/modules/help/help.module
    @@ -81,3 +81,37 @@ function help_preprocess_block(&$variables) {
    +/**
    + * Preprocess function for help-list template
    + *
    + * @param $variables
    + *   An associative array containing:
    + *   - links: A list of links to render.
    + */
    

    Please see https://drupal.org/node/1354#themepreprocess for the standards on preprocess docblocks. Should be something like "Prepares variables for help list templates." and have a "Default template:" line as well.

  2. +++ b/core/modules/help/help.module
    @@ -81,3 +81,37 @@ function help_preprocess_block(&$variables) {
    +    $variables['rows'][] = theme('item_list', array(
    +      'items' => $list,
    +      'attributes' => array('class' => 'help-items'),
    +    ));
    

    Please use a render array like array('#theme' => 'item_list', '#items' => $list, …) instead of calling theme() here. See #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() and #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system.

  3. +++ b/core/modules/help/templates/help-list.html.twig
    @@ -0,0 +1,18 @@
    +{% endif %}
    \ No newline at end of file
    

    Please add a newline (blank line) at the end of the Twig template per https://drupal.org/coding-standards#indenting.

star-szr’s picture

Also while we're at it, since we're using item_list we might as well provide a theme suggestion so that this usage of item_list can be overridden specifically.

Could be as simple as: '#theme' => 'item_list__help'

babruix’s picture

Status: Needs work » Needs review
FileSize
5.92 KB

Problem with tests was because I have added line break in template after </h2>.
So now added "/n" to the test assert as well.

babruix’s picture

Ok, now I see that even after adding "\n" test still fails.
So, here I`ve removed line break in template and test passes.
Maybe it is better to have that line break in template and split test assert into 2 separated lines .
If somebody has better propositions, let me know.

The last submitted patch, 22: theme_help-1986164-22.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: theme_help-1986164-23.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
6.08 KB

Ok, this one should pass tests.

star-szr’s picture

Thanks, that's looking better. Please provide interdiffs when making updates to patches, makes it much easier for reviewers (which we are short on!) :)

These are all pretty minor so leaving at needs review.

  1. +++ b/core/modules/help/templates/help-list.html.twig
    @@ -0,0 +1,20 @@
    +  {% if rows %}
    +    {% for list in rows %}
    +      {{ list }}
    +    {% endfor %}
    +  {% endif %}
    

    We probably don't need the if wrapping the for loop here. I think if we add an 'if' it should be around the div instead.

  2. +++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.php
    @@ -19,52 +19,29 @@ class HelpController extends ControllerBase {
    +        $modules[$module] = $this->l($name, 'help.page',  array('name' => $module));
    

    While we're moving/refactoring this line can we get rid of the extra space before 'array'?

  3. +++ b/core/modules/help/css/help.module.css
    @@ -2,20 +2,12 @@
    -  margin-right: 3%; /* LTR */
    ...
    -.help-items-last {
    -  margin-right: 0; /* LTR */
    -}
    -[dir="rtl"] .help-items-last {
    -  margin-right: 0;
    -  margin-left: 0;
    -}
    

    It seems like we may need to adjust the CSS here rather than just removing it, but I haven't had a chance to manually test the patch. Possibly we could use CSS3 pseudo-selectors now that #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors is in.

jhodgdon’s picture

Actually, regarding item 1 in comment #27, it seems like if the list of topics is empty, the Twig template should not output anything at all, because there are no help topics? Or it should have some "empty" text?

babruix’s picture

Now I think that we probably do not need that if at all, because there are always core modules (which can not be disabled), - so list will never be empty.

jhodgdon’s picture

That is true, and since the Help module itself has help, it should not be necessary to check for the list being empty at all.

babruix’s picture

FileSize
6.05 KB
564 bytes

1)-2) Fixed.
About 3) - as far as I understood we do not need these classes at all anymore.
If we still need them for some reason, then we need to clarify what we want from that CSS.

Status: Needs review » Needs work

The last submitted patch, 31: theme_help-1986164-31.patch, failed testing.

star-szr’s picture

31: theme_help-1986164-31.patch queued for re-testing.

babruix’s picture

31: theme_help-1986164-31.patch queued for re-testing.

star-szr’s picture

Assigned: Unassigned » star-szr

Thanks for bumping this, I wanted to review this one so assigning to myself for that.

babruix’s picture

Status: Needs work » Needs review
star-szr’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs steps to reproduce +CSS
FileSize
63.15 KB
69.76 KB

After manually testing, we do need to make some CSS adjustments otherwise there will be a visual regression from the current behaviour. Attaching a couple screenshots that show a couple breakpoints that look fine with the current markup/CSS. I'm thinking we should either add some @media queries or use CSS3 pseudo selectors rather than reintroduce the .help-items-last class.


+++ b/core/modules/help/templates/help-list.html.twig
@@ -0,0 +1,18 @@
+ * - rows: An array containing list of links to render.

The wording could be improved for this comment, but I also strongly suggest that the variable should be renamed to 'columns'.

Something like this?

columns: Each column contains a list of help links. The help items are split into 4 columns by default.
star-szr’s picture

Assigned: star-szr » Unassigned

Forgot to unassign.

jhodgdon’s picture

As a side effect of #2661200: Make admin/help page more flexible, and list tours on it, the help page will be broken up into sections, and each one will use a Twig template that is part of the Help module, and hence themeable. Working on the patch for that now...

jhodgdon’s picture

Status: Needs work » Closed (duplicate)

I think that the previously-mentioned other issue has now taken care of this.