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';
}
}
Related Issues
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.
Comment | File | Size | Author |
---|---|---|---|
#37 | 1986164-31-after-2.png | 69.76 KB | star-szr |
#37 | 1986164-31-after-1.png | 63.15 KB | star-szr |
#31 | interdiff.txt | 564 bytes | babruix |
#31 | theme_help-1986164-31.patch | 6.05 KB | babruix |
#26 | theme_help-1986164-26.patch | 6.08 KB | babruix |
Comments
Comment #1
plopescHello
Attaching first approach to convert help page rendering to a themable function.
Regards.
Comment #2
babruix CreditAttribution: babruix commentedComment #3
babruix CreditAttribution: babruix commentedComment #5
babruix CreditAttribution: babruix commentedComment #7
babruix CreditAttribution: babruix commentedOk, re-rolled manually instead of using automatic rebase/merge.
Comment #8
disasm CreditAttribution: disasm commentedupdating tags.
Comment #9
star-szrWe'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.
Comment #10
Nigel_S CreditAttribution: Nigel_S commentedComment #11
chanderbhushan CreditAttribution: chanderbhushan commented7: theme_help-1986164-7.patch queued for re-testing.
Comment #12
royalty CreditAttribution: royalty commentedComment #13
royalty CreditAttribution: royalty commentedComment #14
Nigel_S CreditAttribution: Nigel_S commentedCurrently working on conversion of last working patch to Twig template per #9
Comment #15
star-szrUnassigning 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.
Comment #16
jhodgdonI just found another (very old) issue that duplicates this one, which had a D7 patch:
#232466: theme help topics
Comment #17
babruix CreditAttribution: babruix commentedComment #18
babruix CreditAttribution: babruix commentedMy first Twig template :)
Comment #20
star-szrNice! Thanks @babruix. I suspect the test is whitespace sensitive so that may need some tweaking.
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.
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.Please add a newline (blank line) at the end of the Twig template per https://drupal.org/coding-standards#indenting.
Comment #21
star-szrAlso 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'
Comment #22
babruix CreditAttribution: babruix commentedProblem with tests was because I have added line break in template after
</h2>
.So now added
"/n"
to the test assert as well.Comment #23
babruix CreditAttribution: babruix commentedOk, 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.
Comment #26
babruix CreditAttribution: babruix commentedOk, this one should pass tests.
Comment #27
star-szrThanks, 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.
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.
While we're moving/refactoring this line can we get rid of the extra space before 'array'?
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.
Comment #28
jhodgdonActually, 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?
Comment #29
babruix CreditAttribution: babruix commentedNow 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.Comment #30
jhodgdonThat is true, and since the Help module itself has help, it should not be necessary to check for the list being empty at all.
Comment #31
babruix CreditAttribution: babruix commented1)-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.
Comment #33
star-szr31: theme_help-1986164-31.patch queued for re-testing.
Comment #34
babruix CreditAttribution: babruix commented31: theme_help-1986164-31.patch queued for re-testing.
Comment #35
star-szrThanks for bumping this, I wanted to review this one so assigning to myself for that.
Comment #36
babruix CreditAttribution: babruix commentedComment #37
star-szrAfter 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.
The wording could be improved for this comment, but I also strongly suggest that the variable should be renamed to 'columns'.
Something like this?
Comment #38
star-szrForgot to unassign.
Comment #39
jhodgdonAs 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...
Comment #40
jhodgdonI think that the previously-mentioned other issue has now taken care of this.