govfresh / govpress

The WordPress theme for government
https://wordpress.org/themes/govpress/
GNU General Public License v2.0
135 stars 50 forks source link

Code Review of Icon Menu #21

Closed devinsays closed 10 years ago

devinsays commented 10 years ago

I've added an "Icon Menu" which can be displayed above the content section. It uses a custom walker to apply the class set in the WordPress menu admin to the actual link.

If you set an icon class for a menu item, that icon will appear above the text link. These were the icons used in the original version of the theme: .icon-calendar .icon-gavel .icon-bar .icon-comment .icon-envelope

The full list of available icons is here: https://github.com/govfresh/govfreshwp/blob/master/fonts/font-awesome/font-awesome.css

The menu also has function to count the number of menu items and apply that as a class to the container so that widths can be applied via CSS.

This is perhaps the best method to do it. Perhaps not. Any recommendations would be appreciated.

How should visibility options be added? (e.g. only display on home page, only display on search page, etc.)

Should this functionality be moved into an add-in plugin instead? Is it too custom for general use?

devinsays commented 10 years ago

The css classes for item count aren't working correct.

devinsays commented 10 years ago

I simplified this by using filters rather than a walker: https://github.com/govfresh/govpress/blob/master/inc/icon-menu-extras.php

Still not completely happy with it as someone might intentionally want to use icon-* on another menu.

Perhaps the best solution is to just add an additional rule to all the icon css:

.menu-icon .icon-* a:before { content: '\icon' }

But it's also annoying to add all that css.

lukefretwell commented 10 years ago

Looks like you're using an older version of FA. Do we want to use the newer one?

http://fontawesome.io/

lukefretwell commented 10 years ago

Added icon CSS into menu, and it's breaking the spacing, causing each to block.

See here: http://civicmeet.govfresh.com/

devinsays commented 10 years ago

I'm using version 4.0.3 of FontAwesome- which looks like the latest. Is there something that makes you think it's not?

lukefretwell commented 10 years ago

I tried using the newsest FA code is the CSS and it didn't work so I went with the old form and that did.

On Saturday, February 15, 2014, Devin Price wrote:

I'm using version 4.0.3 of FontAwesome- which looks like the latest. Is there something that makes you think it's not?

Reply to this email directly or view it on GitHubhttps://github.com/govfresh/govpress/issues/21#issuecomment-35159807 .

Luke Fretwell 415.722.8678 luke@lukefretwell.com http://lukefretwell.com

devinsays commented 10 years ago

Could you give me an example of the new form and old form?

I did alter the CSS slightly. I'm using "icon" rather than "fa". https://github.com/govfresh/govpress/blob/master/fonts/font-awesome/font-awesome.css

lukefretwell commented 10 years ago

I see. I had to use 'icon-rocket' which is similar to the markup from 3.2.1 to get it to work so assumed you were pulling from those.

devinsays commented 10 years ago

I'm leaning towards changing the markup back to "fa" from "icon". That could help prevent conflicts with other icon libraries. Also allows us to link straight to the font-awesome page so folks can find the class they need. This will cause issues on some of the staging sites- but should be done before release if it is going to happen. Thoughts @lukefretwell?

lukefretwell commented 10 years ago

+1

On Saturday, February 22, 2014, Devin Price wrote:

I'm leaning towards changing the markup back to "fa" from "icon". That could help prevent conflicts with other icon libraries. Also allows us to link straight to the font-awesome page so folks can find the class they need. This will cause issues on some of the staging sites- but should be done before release if it is going to happen. Thoughts @lukefretwellhttps://github.com/lukefretwell ?

Reply to this email directly or view it on GitHubhttps://github.com/govfresh/govpress/issues/21#issuecomment-35819698 .

Luke Fretwell 415.722.8678 luke@lukefretwell.com http://lukefretwell.com

devinsays commented 10 years ago

Icon prefix has been changed back to "fa". You'll need to update any icon menus you've already set.