Replace help page layout CSS with reuseable layout classes

Patch added.

Re-rolled.

Thanks @LewisNyman for pointing that out! I attached a new patch which fixes that.

HEAD was broken. back to rtbc.

But this means that the help screen only looks good in seven since the quarter class only exists in seven's layout.css

Since this is changing module CSS, can we also get testing in e.g. Stark/Bartik in addition to Seven?

This patch is going to conflict with #2351991: Add a config entity for a configurable, topic-based help system, and on that patch, I was asked to use an inline template... maybe we should combine forces?

The other issue #2351991: Add a config entity for a configurable, topic-based help system has been postponed to 8.1.x. So we should move forward with this one ASAP.

So... The other thing is that on #2351991: Add a config entity for a configurable, topic-based help system I was asked for that patch (which I did) to use an inline template for building the help page. Should we combine that idea with this issue? It affects the same area of the code. Or, we could use a real template for it, which would probably be even better?

The part of the patch on that issue that does this is in HelpController::helpMain()... We would need to do something like this:

-      '#markup' => '<h2>' . $this->t('Help topics') . '</h2><p>' . $this->t('Help is available on the following items:') . '</p>' . $this->helpLinksAsList(),
     );
+
+    $template = '<h2>{{ title }}</h2><p>{{ header }}</p>{{ links }}';
+
+    $output['modules'] = array(
+      '#type' => 'inline_template',
+      '#template' => $template,
+      '#context' => array(
+        'title' => $this->t('Help topics'),
+        'header' => $this->t('Help is available for the following modules:'),
+        'links' => array('#markup' => $this->helpLinksAsList()),
+      ),
+    );

And then in helpLinksAsList() we would need to do something like this:

-        $modules[$module] = $module_info[$module]->info['name'];
+        $modules[$module] = array(
+          'title' => $module_info[$module]->info['name'],
+          'url' => new Url('help.page', array('name' => $module)),
+        );
       }

and then one other section below:

-    foreach ($modules as $module => $name) {
-      $output .= '<li>' . $this->l($name, new Url('help.page', array('name' => $module))) . '</li>';
+    foreach ($modules as $info) {
+      $output .= '<li>' . $this->l($info['title'], $info['url']) . '</li>';

This seems related to "making the help page more standardized" but if you think it should be a separate issue, we can do that too.

Yeah the other issue got postponed, and it does a lot more.

So... Let's step back a bit. The Help page (with or without the new topic entities) has a section that makes a 4-column list (on wide browsers, which hopefully degrades smoothly to a two-column then one-column list in narrower browsers; I think that works).

It is currently doing that in HelpController::helpLinksAsList() by making straight markup: it outputs

<div class="clearfix"><div class="help-items"><ul>

then iterates through the list of modules, outputting a <li></li> for each one, and after each 25% of the list is over, it outputs

</ul></div><div class="help-items-2"><ul>

(changing that div class to help-items-3 and help-items-last for the other two columns). And then it closes the UL and DIVs at the end.

So. This is bad -- a Controller should be returning render arrays, not HTML. As one step towards making it slightly better, as one small part of the patch on #2351991: Add a config entity for a configurable, topic-based help system, I was asked to use an in-line template, which I did (see comment #20 above on this issue).

IMO a better fix would be to define something like a "four-column-item-list" #theme or #type, and use that, rather than just moving the CSS to something more generic like what this patch does. This patch doesn't change the fact that the Help Controller is outputting its own HTML and the theme cannot change it in any way.

Thoughts? If that is too out of scope, then this change seems like a fine idea, but ... it just doesn't go very far towards fixing the underlying badness of the Help module's rendering here.

And don't worry about any rerolls on the other issue. It's been moved to a contrib module for the time being and if we do something here I'll apply the changes there.

Hm. We have a type for item list. Maybe it can have an added option for 4-column?

What CSS fix are you advocating? Could we at least make each of the 4 columns into a render array with #theme = 'item-list' instead of having the code output literally the UL and LI tags, which is horrible?

@jhodgdon I agree with you a new '#type' => 'columns' render element would be ideal, but I think you will agree that is out of scope for this issue :). Basically this issue is just a followup for #2017257: Create generic layout classes to apply the generic css classes to the Help module. Let's keep this issue focused on that target to make the css in core more maintainable.

I took at a look at your patch in #110 of #2351991: Add a config entity for a configurable, topic-based help system and there is indeed an of overlap in HelpController::helpLinksAsList() and HelpController::helpMain(). I rewrote the hard coded html to a renderable array, albeit not a very pretty one, that should be easy to merge into #2351991: Add a config entity for a configurable, topic-based help system. I'll go have a look around to see if there is any effort going to create a 'columns' type of element to make this type of display easier to create in the future.

I don't get it. I was suggesting that the code to generate the link list be changed to at least use theme item list or something like that, was told it's out of scope, but that's what this patch does. Thanks! It's at least not so horrible as it was. :)

What can I say, I aim to please :)

I'm not sure on the way forward regarding the <li class="_"> @LewisNyman mentioned in #28. I expected different markup from the renderable array. I opened a new issue at #2402165: #theme => 'links' renders <li class="_"> when the #links array is not associative to see what is up.

Yeah. Good job reporting that other issue... will comment there. But according to template_preprocess_links(), #links is supposed to be an associative array, so probably we should make it one.

I swapped '#theme' => 'links' with '#theme' => 'item_list' for now, since it generates less markup. Quite ironic since the reason #theme => links was invented in the first place was to reduce markup..

Well, that's at least no worse than the current code it is replacing!

I haven't tried it out to see if the classes/markup are OK.

Code looks good... Has someone verified that this still works on different-width browsers?

Thanks! It looks like this satisfies the goals of the issue, and the code is at least no worse than what is currently in help.module. I think we should commit it. But before we do we need a Beta Evaluation added to the issue summary.
https://www.drupal.org/contribute/core/beta-changes

Great! Should be ready to go then.

Committed f6c5660 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.