Hey,

I'v rolled out a patch with some extra functionality for your module. Here's what I've done:

  • Removed install file: Why use a separate table when you can use the menu_links table. Store the icon path in menu_link's options column.
  • Added 'icon folder', 'size' and 'position' to the configuration settings page.
  • Added upload file field for the icons.
  • Removed a lot of theme function that aren't needed IMO.
  • Added caching.

Let me know how you feel about these changes.

CommentFileSizeAuthor
#2 menu_icons.patch17.31 KBskilip

Comments

vm’s picture

vm commented
Status: Active » Postponed (maintainer needs more info)

you did not attach the patch

skilip’s picture

skilip commented
StatusFileSize
new menu_icons.patch17.31 KB

I haven't figured out how to create a patch which also removes both 'menu_iccons.install' and 'menu_iccons.css'. I hope the patch is further ok.

grendzy’s picture

grendzy commented
Status: Postponed (maintainer needs more info) » Active

My apologies for not responding sooner; I haven't looked at this in a while. I'm subscribed to this queue now so I should be more responsive in the future. ;-) I appreciate your patch and I'll look at it soon.

These sound good. I'll be glad to be rid of all those extra theme functions if possible; at the time I couldn't figure out a way to alter menu items (adding the menu-$mlid class) without duplicating the whole stack (most of that stuff was copied from nice_menus which faced a similar problem). I would love it if all that junk could be replaced with a simple preprocess function.

Also this module will work better now that #326210: (Needs tests) menu_submit & mlid on menu creation has landed in D6.

skilip’s picture

skilip commented

Nice seeing some activity again ;)

grendzy’s picture

grendzy commented

I haven't tested the patch, but just skimming the code things are looking pretty good.

I'm not sure I agree with removing the CSS file. The intent was to save some bandwidth with tons of repeated background-repeat: no-repeat; for example, and also to have a stylesheet in place that themers can override.

What are your thoughts on this? I'm usually not worried about piling on additional CSS files because most of my projects are able to use the CSS aggregator. Though I'm aware that folks using private files (which I occasionally need too) can't use that feature, and IE is limited to 30 style sheets.

skilip’s picture

skilip commented

A css file is generated dynamically and cached when possible, so there should be no concern there ;).

skilip’s picture

skilip commented

For your concerns on the background-repeat repetition:

IMO adding an extra stylesheet just for that (and maybe padding and a few others) is giving us more overhead tha having that declaration per menu-item. Another argument for declaring that per item, is having the possibility of having pore specific styles per item in the future. Think of adding a configuration option for choosing where to align the icon on a per-item level.

skilip’s picture

skilip commented
Version: » 6.x-2.0
Status: Active » Closed (fixed)