ContextInstitute / bfcom

http://bfn.context.org
GNU General Public License v3.0
4 stars 1 forks source link

Created top menu system to replace joints_top_nav #90

Closed DavidScottBernstein closed 5 years ago

DavidScottBernstein commented 5 years ago

Fixed #83, also fixed #76. Created function (bfc_top_nav()) to replace call to joints_top_nav() in nav-bfc-top.php to build the main menu. In style.css, emoved old way of adding icons to menu and changed media query to remove text on narrow windows.

DavidScottBernstein commented 5 years ago

@rgilman @iangilman, I've been making good progress on the PHP top menu generator. I'm implementing it using a template method pattern with an embedded decorator. This will give us maximum flexibility with minimal effort.

I removed the menu item icon code that Ian and I added to style.css and updated the media query, @media (max-width:767px) to remove the menu item text when the window becomes narrow.

The highlight margin is too big. Please make suggestions on styles, borders, margins, etc. Also, it's easy to add more capabilities to the function. Just let me know what else you'd like it to do and I'll tweak the template method.

PS. I'll be out all next week for Thanksgiving. I'm back on 11/26 for the week and then teaching for a client the following week (12/3-7).

rgilman commented 5 years ago

@DavidScottBernstein, this looks great! One tweak I suggest is to redo all the IDs away from menu-item-### to bfc-topnav-home, bfc-topnav-members, etc.. That way we can avoid conflicts with the WP menu system at some future point.

DavidScottBernstein commented 5 years ago

@rgilman, I changed top nav menu IDs from 'menu-item-xxx' to 'bfc-topnav-xxx' and no longer add the ID as a selector.

rgilman commented 5 years ago

@DavidScottBernstein, looks good. BTW, I'd say that you "no longer add the ID as a class" since IDs and classes are both selectors. In any case, I think the change is a good one.

@iangilman, I'm ready to have this merged.

iangilman commented 5 years ago

… And thanks for the heads up on your availability!

DavidScottBernstein commented 5 years ago

All issues on David23 have been resolved. I'm assuming that my reviewer (you) should hit the "Resolve conversation" button. Let me know if you want me to click it instead.

DavidScottBernstein commented 5 years ago

Oops, I forgot one change. You're probably still at lunch. I'll post another comment here when I finished the last patch.

DavidScottBernstein commented 5 years ago

Okay, everything is done for this branch. Please review.

iangilman commented 5 years ago

We don't need to resolve the individual conversations... I've approved and merged the pull request; that's enough. :-)

DavidScottBernstein commented 5 years ago

Great, I'm glad everything looks good. And it was fun to use some GoF patterns in the process.