Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Code Review of Icon Menu #21

Closed
devinsays opened this issue Feb 10, 2014 · 11 comments
Closed

Code Review of Icon Menu #21

devinsays opened this issue Feb 10, 2014 · 11 comments

Comments

@devinsays
Copy link
Contributor

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
Copy link
Contributor Author

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

@devinsays
Copy link
Contributor Author

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.

@devinsays devinsays reopened this Feb 13, 2014
@lukefretwell
Copy link
Member

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

http://fontawesome.io/

@lukefretwell
Copy link
Member

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

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

@devinsays
Copy link
Contributor Author

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
Copy link
Member

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//issues/21#issuecomment-35159807
.

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

@devinsays
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

+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//issues/21#issuecomment-35819698
.

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

@devinsays
Copy link
Contributor Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants