search

cuny-academic-commons / cbox-theme

Default theme for Commons In A Box
GNU General Public License v2.0
20 stars 15 forks source link

Background color does not stick when background overlay is used #161

Open christianwach opened 11 years ago

christianwach commented 11 years ago

@BoweFrankema before I do a pull request, could you please review the menu restyling that I've done on:

http://www.paulschacht.net/

It's my assumption that this is what the menus (both base and main) were supposed to look like, but that some odd CSS declarations crept in somewhere.

My update removes the (too generous) padding, lines up submenus correctly, makes the spans block elements (so the rollover state isn't lost when the page title wraps and you hover between the lines of the title) and extends the links to the full width of the menus and submenus so there aren't any inactive regions. It also leaves the "active trail" items in their highlighted state.

Cheers, Christian

christianwach commented 11 years ago

@BoweFrankema I should have pointed you to the commit too, I guess! 7f85ef61603298041a8665d1f5df59c07e526885

christianwach commented 11 years ago

I'll keep adding some further notes on issues as I stumble across them...

When the Background Color option is set for a menu (in this case the Below Header Menu), it appears that Infinity writes some CSS into 'dynamic.css'. However, it has no effect, because setting the background-color alone is not enough. The background-image declaration has to be cleared for the background-color to show. The CSS to so this properly would therefore be:

body.theme-option .sub-menu {
  background-color: #ff006f; /* my chosen bg-color */
  background-image: none; /* clear gradient */
}

For Sub-item Backgrounds, things get more complex, because the anchors themselves have their background-image set to:

.base-menu ul li.current-cat a, 
.base-menu ul li.current_page_item a, 
.base-menu ul li.current-menu-item a {
  font-weight: bold;
  background: url('/wp-content/themes/cbox-theme/assets/images/bg-body.gif') repeat transparent;
}

Therefore, the override has no effect as it is targetting the wrong element:

body.theme-option .sub-menu ul ul {
  background-color: #62ff00;
}

I've had a quick look at Infinity_Options_Policy but can't figure out where these CSS overrides are being built. I guess this is one for another time, given that I can adapt the theme with my own CSS, but I'd imagine people who want to use the Theme Options might be frustrated that they have no effect.

r-a-y commented 11 years ago

@christianwach - Also see my related ticket on the Infinity repo: https://github.com/PressCrew/infinity/pull/83

r-a-y commented 10 years ago

Okay gang, so how do we want to solve this?

My fix in https://github.com/PressCrew/infinity/pull/83 works. The only issue is the menus don't really gel after applying a custom background color.

See the Home button after the background color is altered to red. I guess this is a user CSS issue more than anything.

2014-05-20_172051

christianwach commented 10 years ago

@r-a-y As far as I can tell, your fix only works to a degree. I've spent a while on this and I'm fairly sure now that there are deeper issues at play here. The CSS from the "Background Color" and "Sub-item Background" options suffer from the same issue I mention in https://github.com/cuny-academic-commons/cbox-theme/pull/198#issuecomment-43786030 in that they generate declarations that are:

"Well if I were you, I wouldn't be starting from here" as the old joke goes...

r-a-y commented 10 years ago

Thanks for looking into this, @christianwach.

I didn't even see the "Sub-item Background" setting. (Too many options!) I have a potential fix for this.

In /cbox-theme/engine/config/features.ini - change line 466 to

style_selector = "#sub-menu-wrap ul li.current-menu-item > a, #sub-menu-wrap ul a:hover"

Needs a bit more work.

This addresses the "Home" button background colour as well. I haven't looked into the background-image issue, but the overlay and background colour settings worked okay for me.

christianwach commented 10 years ago

@r-a-y Nice find - there's definitely progress to be made by tweaking the selectors in that file. I've tried a few variations there, but cannot come up with a solution that works for all combinations of options. Indeed, I'm now confused about the intent of the options... either there are too many or too few to be logical:

"Font Color" and "Font Weight" apply to all items in a menu, while "Background Color" only applies to the menu "bar" (red in your screenshot) and not the background of the dropdowns, which have

background: url('/wp-content/themes/cbox-theme/assets/images/bg-body.gif') repeat transparent;

applied to them in layout.css by default. This, of course, overrides any colour subsequently applied to them. With your fix applied, "Sub-item Background" sets the hover colour of menu items as well as the current item but breaks when you also set "Background Overlay" unless you clear the "Background Color". I could go on, but this rabbit hole goes deep.

I think the "too few" or "too many" options issue is perhaps the key here. At present, people may expect the options to do different things because the options themselves are poorly defined. It seems to me (assuming that the current set of options remains in place) that the menu options should break down thus:

Each menu has base properties such as:

Each menu then also has the following ancestry properties:

This may not be an exhaustive list. My point is that the current set of options do not reflect the actual functioning of the menus and that this leads to the CSS rule confusion. And then there's that pesky .sub-menu naming collision, which complicates things even further...

MrMaz commented 10 years ago

I wouldn't be against dropping these complex options from Infinity completely. It's apparent that either the original/older Infinity design was more compatible with these settings or simply weren't tested thoroughly enough. I would like to completely replace the superfish menu with something else in the future anyways, at which point we can rethink this from the ground up.

Over time with feedback about Infinity coming in, it's become very clear to me that "forcing" end users to learn a bit of CSS if they want to get very specific is quite a bit less painful than trying to cover all cases with a GUI type solution.

I'm not trying to throw your work so far out the window. I'm on board either way. Just wanted to let you guys know that I'm not married to these settings ;)

christianwach commented 10 years ago

@MrMaz I wholeheartedly agree!

boonebgorges commented 10 years ago

Over time with feedback about Infinity coming in, it's become very clear to me that "forcing" end users to learn a bit of CSS if they want to get very specific is quite a bit less painful than trying to cover all cases with a GUI type solution.

Yes :)

@r-a-y @christianwach I've gotten a bit lost trying to parse out the possible solutions presented above. I definitely get the sense that there are a number of options - overlay, color, and each of these for the sub-items - and that actually "fixing" this issue is likely to involve rethinking the level of customizability that we actually want to provide here. But for the purposes of the maintenance release, that kind of rethinking is not viable, so let's instead do one of the following:

  1. Call it a "known bug" and say wontfix
  2. If there's a way that we can make this even more of an edge case by fixing it for top-level items (while leaving a bug in place for sub-items), let's do it. I get the sense that this is what @r-a-y's fix boils down to.

Thanks!

r-a-y commented 10 years ago

If there's a way that we can make this even more of an edge case by fixing it for top-level items (while leaving a bug in place for sub-items), let's do it.

I went with this option :)

Going to leave this issue open though for the time being.