Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The help page implements a custom four column layout in CSS.
Proposed resolution
Remove the custom CSS and replace with the reuseable classes introduced in #2017257: Create generic layout classes
Remaining tasks
Write patch
Test
User interface changes
None
API changes
None
Beta phase evaluation
Issue category | Task because code clean up |
---|---|
Unfrozen changes | Unfrozen because it only changes mark up and CSS |
Comment | File | Size | Author |
---|---|---|---|
#43 | Screen Shot 2015-01-10 at 18.32.35.jpg | 320.94 KB | LewisNyman |
#40 | 2337317-40.patch | 4.12 KB | LewisNyman |
#40 | interdiff.txt | 916 bytes | LewisNyman |
#36 | 2337317-36.patch | 3.99 KB | LewisNyman |
#36 | interdiff.txt | 1.02 KB | LewisNyman |
Comments
Comment #1
ngocketit CreditAttribution: ngocketit commentedComment #2
ngocketit CreditAttribution: ngocketit commentedPatch added.
Comment #4
ngocketit CreditAttribution: ngocketit commentedComment #5
ngocketit CreditAttribution: ngocketit commentedRe-rolled.
Comment #6
LewisNymanThanks, one small thing. We don't actually need the layout-container class here. It indents everything slightly:
BEFORE:
AFTER:
Comment #7
ngocketit CreditAttribution: ngocketit commentedThanks @LewisNyman for pointing that out! I attached a new patch which fixes that.
Comment #8
LewisNymanLooks perfect! A tiny shift in layout now but that's consistency for you ;-)
BEFORE:
AFTER:
Comment #9
LewisNymanComment #12
herom CreditAttribution: herom commentedHEAD was broken. back to rtbc.
Comment #13
alexpottBut this means that the help screen only looks good in seven since the quarter class only exists in seven's layout.css
Comment #14
LewisNyman@alexpott Ok thanks, if that's the aim then we can move forward on #2298821: Move generic layout styling into system.admin.css
Comment #15
LewisNyman#2298821: Move generic layout styling into system.admin.css is now in so back to RTBC
Comment #16
webchickSince this is changing module CSS, can we also get testing in e.g. Stark/Bartik in addition to Seven?
Comment #18
jhodgdonThis 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?
Comment #19
jhodgdonComment #20
jhodgdonThe 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:
And then in helpLinksAsList() we would need to do something like this:
and then one other section below:
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.
Comment #21
LewisNyman@jhodgdon I'm happy to merge efforts here, but #2351991: Add a config entity for a configurable, topic-based help system looks like a big patch. It might be better to keep these objectives separate to make it them more achievable. The current patch in this issue is dead simple. Happy to reroll either issue.
Comment #22
jhodgdonYeah 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
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(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.
Comment #23
LewisNymanhmm we could do that, I guess we should create a #type for item list? I still think we should use the generic admin CSS, we don't want core or contrib modules to have to manage their own layout CSS, it would be a pain to keep them consistent.
Comment #24
jhodgdonHm. We have a type for item list. Maybe it can have an added option for 4-column?
Comment #25
LewisNymanI think the scope of the issue may of got a little overloaded. How about we move the inline markup vs template into a a follow up issue? It's easy for me to write CSS and add classes, this issue can be a quicker win.
Comment #26
jhodgdonWhat 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?
Comment #27
idebr CreditAttribution: idebr commented@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()
andHelpController::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.Comment #28
LewisNymanYep. Exactly. I'm not saying that we shouldn't fix the way the markup is generated but I think we should split them.
I noticed that a class of just an underscore is being added to the li's. This doesn't happen in HEAD.
Comment #29
jhodgdonI 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. :)
Comment #30
idebr CreditAttribution: idebr commentedWhat 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.Comment #31
jhodgdonYeah. 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.
Comment #32
idebr CreditAttribution: idebr commentedI 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..
Comment #33
jhodgdonWell, 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.
Comment #34
LewisNymanThe markup looks good to me. I noticed that we are deleting all the CSS which means we can remove the file and the libraries.yml. We should be good to go.
Comment #36
LewisNymanComment #40
LewisNymanI think i removed too much.
Comment #41
LewisNymanComment #42
jhodgdonCode looks good... Has someone verified that this still works on different-width browsers?
Comment #43
LewisNymanYep, the layout classes are responsive:
Comment #44
jhodgdonThanks! 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
Comment #45
LewisNymanDone
Comment #46
jhodgdonGreat! Should be ready to go then.
Comment #47
alexpottCommitted f6c5660 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.