Posted by jenlampton on September 8, 2012 at 6:58am
9 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | theme system cleanup, Twig |
Issue Summary
theme_filter_tips currently does this janky thing to add it's own classes to both the containing div for each filter, and the LI tags for each tip.
Can we re-work this theme function to use the new Attributes as per
#1290694: Provide consistency for attributes and classes arrays provided by template_preprocess()
And while we're at it, can we please call theme('item_list) instead of writing our own UL and LI tags in this theme function?
Comments
#1
I've got a small piece of code now that replaces the custom LIs with a call to theme('item_list') but I don't understand the overarching goal of the rest of this.
Would you like to rework this into a preprocess and a template file, in preparation for migration to twig templates? Or just rewrite the lines that add the wrapper divs , in order to take advantage of the new Attributes class? If its the later, then I don't understand how rewriting a simple line of code like:
$output .= '<div class="compose-tips">';into something that uses the Attributes class, a la$output .= '<div' . new Attribute(array('class' => array('compose-tips')) . '>';would make this any cleaner for themers.IMHO, the obvious problem with this theme function that makes it really ugly and hard to read and comprehend are the various conditions ($multiple, etc) wrapped inside nested foreach loops.
PS. I'm new to the Drupal 8 retheming movement and twig movement, but exited to get started helping, so go easy on me :)
#2
To backup what i mentioned in #1, here is the patch.
#3
#4
+++ b/core/modules/filter/filter.pages.incundefined@@ -67,11 +67,17 @@ function theme_filter_tips($variables) {
}
- $output .= '</ul>';
+ $output .= theme('item_list', array(
+ 'items' => $items,
+ 'attributes' => array('class' => array('tips'))
I think this should use a __suggestion for item_list__filter_tips or so ...
Overall: Nice cleanup. Leaving for other to review more.
#5
#4 makes sense to me.
#6
This looks better, but does
item_list__filter_tipsactually work? I think theme_item_list will need to support this suggestion before this will work.See #1751194: Introduce hook_template_suggestions_HOOK to separate suggestions from variable preprocessing
#7
Postponing on #891112: Replace theme_item_list()'s 'data' items with render elements
That patch is pretty much ready and just needs a commit. Afterwards, we'll convert the $items here into the new structure.
#8
Since that issue is now fixed, reopening this one :)
#9
.
#10
This patch to d8 core head outputs:
<div class="item-list"><ul class="tips"><li class="odd first"><div class="item-list"><ul><li class="odd first last">filter-filter-url</li></ul></div></li><li class="even"><div class="item-list"><ul><li class="odd first last">filter-filter-html</li></ul></div></li><li class="odd last"><div class="item-list"><ul><li class="odd first last">filter-filter-autop</li></ul></div></li></ul></div>Which when rendered in the admin overlay looks wrong to the end user too.
An error message is returned a number of times:
User error: "data" is an invalid render array key in element_children() (line 6022 of core/includes/common.inc).#11
See here for the new syntax for item lists:
http://drupal.org/node/1842756
( there is also some code that can be used for item-list.twig )
#12
#13
Following on @jwilson3's work, I've
'filter_tips'entry fromfilter_theme()theme_filter_tipsapplies only to the Compose tips page (at /filter/tips), merged everything fromtheme_filter_tipsintotheme_filter_tips_long--the page callback for "Compose tips", usingtheme('item_list__filter_tips_long')instead of building our ownul.theme('filter_tips')(and only using a small part of it), calledtheme('item_list__filter_tips_guidelines')fromtheme_filter_guidelines--the theme callback that produces the tips for each format below textarea fields.This is a general theme sytem cleanup task that will also benefit Twig by allowing us to re-use item-list templates when we convert these to preprocess function + twig files.
#14
item_list__filter_tips_guidelines reads and sounds a little weird.
Couldn't we keep it as analog to the old format as possible: item_list__filter_tips and item_list__filter_guidelines. Other than that, the patch looks good, thanks for forward movement steve.
#15