I am modifying the book.module to emit ordered lists (instead of unordered) for the menus, but this does not work because system.module forces all li.leaf elements to be drawn with a bullet. The following patch fixes it,

--- a/modules/system/system.css
+++ b/modules/system/system.css
@@ -166,19 +166,19 @@ ul.menu {
 ul.menu li {
   margin: 0 0 0 0.5em;
 }
-li.expanded {
+ul li.expanded {
   list-style-type: circle;
   list-style-image: url(../../misc/menu-expanded.png);
   padding: 0.2em 0.5em 0 0;
   margin: 0;
 }
-li.collapsed {
+ul li.collapsed {
   list-style-type: disc;
   list-style-image: url(../../misc/menu-collapsed.png);
   padding: 0.2em 0.5em 0 0;
   margin: 0;
 }
-li.leaf {
+ul li.leaf {
   list-style-type: square;
   list-style-image: url(../../misc/menu-leaf.png);
   padding: 0.2em 0.5em 0 0;

though it is suboptimal because one should have used ul>li.leaf (not supported by IE6).

CommentFileSizeAuthor
#21 223341-menu-css.patch1.14 KBcasey
#14 223341-menu-css.patch1.01 KBkars-t
#6 menus-css.patch1.71 KBjody lynn
#3 drupal-head-223341.patch810 bytesbonzinip

Comments

bonzinip’s picture

bonzinip commented
Status: Active » Reviewed & tested by the community

the patch is present in the description

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Version: 5.1 » 7.x-dev
Status: Reviewed & tested by the community » Needs work

Hey, Paolo,
thanks for your contribution - this change seems to make sense!
However, this is no usable patch (see Creating patches), and without testing, it is certainly not RTBC. Furthermore, bugs get fixed in HEAD (which is now D7) and are backported to D6 afterwards.
Please take this into account and create a new patch, then I will be the first one who tests it...
Best regards, Pancho

bonzinip’s picture

bonzinip commented
Status: Needs work » Needs review
StatusFileSize
new drupal-head-223341.patch810 bytes

I didn't create the patch from CVS, but I can confirm that the patch works (I have it on http://smalltalk.gnu.org/step for example). Here is a patch for HEAD instead.

dvessel’s picture

dvessel commented
Status: Needs review » Needs work

bonzinip, the patch has to be done from the root install with a checkout from CVS. You also didn't account for the selectors in the RTL style sheet.

cvs diff -up > nameovpatch.patch

But.. I think a better solution to this is to move those menu styles (they are part of the menus system) into system-menu.css so theme devs can override the whole style sheet from their themes instead of worrying about how selectors are formed.

bonzinip’s picture

bonzinip commented

The patch works and is based on the CVS repo, it should not matter how I created it. :-)

The RTL style sheet was not affected, but I might have been wrong. Maybe some settings like the padding actually *do* belong in a more general"li.something" rule; only the list-styles should be changed to "ul.menu li.something".

I don't know enough to understand the last part of your reply. Are you taking the duty?

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
StatusFileSize
new menus-css.patch1.71 KB

New patch handles both RTL and LTR

brianV’s picture

brianV commented
Status: Needs work » Needs review

just bumping for testbot, although this should work...

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Title: menus cannot be created using <ol> » <ol>
Priority: Normal » Minor
jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Title: <ol> » menus cannot be created using <ol>
kars-t’s picture

kars-t commented
Status: Needs review » Reviewed & tested by the community

This is a nice small patch.
It combines li.expanded, li.collapsed, li.leaf common stiles to one declaration and makes sure only ul gets the disc image set.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD, thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs work 
+++ modules/system/system-menus.css	7 Mar 2009 21:05:27 -0000
@@ -8,21 +8,19 @@ ul.menu {
+  list-style-image: url(../../misc/menu-expanded.png); ¶

1) Trailing white-space here.

2) This patch broke menu bullet icons/images in Garland.

Powered by Dreditor.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Component: system.module » markup
Priority: Minor » Normal
kars-t’s picture

kars-t commented
Status: Needs work » Needs review
StatusFileSize
new 223341-menu-css.patch1.01 KB

Made a new patch that fixes the trailing space and changed garlands CSS.

imho Garland now should use the specific ul li.leaf path so it has the same weight and can override system.css.

Status: Needs review » Needs work

The last submitted patch, 223341-menu-css.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

#14: 223341-menu-css.patch queued for re-testing.

cosmicdreams’s picture

cosmicdreams commented

What would be the best way to test this patch? Can anyone help me setup a proper test of this?

kars-t’s picture

kars-t commented

@cosmicdreams
Good question. In Garland you can just look at the navigation menu with firebug if it shows the correct images. But I don't know if this is a really good test for it. And basically "all" themes should be checked if they need to be patched to show their icons. I don't know if there are already simpletests for themes. Even if you couldn't check CSS inheritance than maybe you could at least check for a CSS class that can override the images.

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs review » Reviewed & tested by the community

Just tested #14 and it looks good. Thank you :)

realityloop’s picture

realityloop commented

#14: 223341-menu-css.patch queued for re-testing.

casey’s picture

casey commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 223341-menu-css.patch1.14 KB

Reroll. Stil RTBC.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
dries’s picture

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

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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