theme_filter_tips is one of those rather neglected theme functions that really needs to revamped - I know its late and could be bumped to D8, however its also a pretty easy change to make this 1) more accessible and 2) actually themeable. Right now its not really themeable because there are no classes on the UL elements, so its indistinguishable from any other UL (for all intensive purposes).

I propose we change it to have DIV wrappers, proper heading levels and classes to make it all nice and themeable with CSS only.

This cropped up after I was trying to theme the compose tips page for Bartik and found it was not really doable because of the lack of classes and the crappy structure - I thought about overriding the theme function but that seems weird that our core theme has to override a theme function just to be able to theme something?

I'm putting this in as a bug because the structure of this is not great and should have proper heading levels (we just did this for the short tips that appear below form elements, seems to follow on that the compose tips page should be the same).

Posting a patch for review and discussion.

CommentFileSizeAuthor
#5 theme_filter_tips_3.patch1.19 KBJeff Burnz
#5 after-patch-theme_filter_tips.png135.12 KBJeff Burnz
#2 theme_filter_tips_2.patch1.19 KBJeff Burnz
theme_filter_tips.patch1.15 KBJeff Burnz

Comments

Jeff Burnz’s picture

Jeff Burnz commented
Status: Active » Needs review

bot...

Jeff Burnz’s picture

Jeff Burnz commented
StatusFileSize
new theme_filter_tips_2.patch1.19 KB

Dammit, wrong patch - this is the one...

Jeff Burnz’s picture

Jeff Burnz commented 
+++ modules/filter/filter.pages.inc	12 Sep 2010 23:11:11 -0000
@@ -55,17 +55,17 @@
+        $output .= '<div class="filter-type' . ' filter-' . drupal_html_class($name) . '">';

Even I can see that this should be...

   $output .= '<div class="filter-type filter-' . drupal_html_class($name) . '">';

Powered by Dreditor.

moshe weitzman’s picture

moshe weitzman commented

Is there some way to get the multiple case to use the same markup as the single case. Not a big deal.

Jeff Burnz’s picture

Jeff Burnz commented
StatusFileSize
new after-patch-theme_filter_tips.png135.12 KB
new theme_filter_tips_3.patch1.19 KB

I started out going that route but thought that might be too much to get through for D7 so punted for a more simple markup change, something to think about though.

Cleaning up my patch and adding an "after" screenshot (bartik).

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

better

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

I've no problems with this patch and it cleans up a few things. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Needs accessibility review

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Needs accessibility review

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.