Skip to:
Content

Opened 3 years ago

Closed 3 years ago

#2691 closed enhancement (fixed)

Default Theme - menu support

Reported by: DJPaul Owned by: DJPaul
Milestone: 1.5 Priority: normal
Severity: Version:
Component: Theme Keywords: has-patch, needs-testing
Cc:

Description

Let's add custom navigation menu support to BP-Default. I have put a first pass into the trunk, but desperately need help with the CSS.

Attachments (4)

changeDepthParam.patch (558 bytes) - added by hnla 3 years ago.
minor adjustment to wp_list_pages depth Parem from 1 to 0
Add_sfhover_class_to_wp_list_pages.patch (655 bytes) - added by hnla 3 years ago.
add #nav li items to existing sfhover jquery for adminbar
2691-2.patch (3.5 KB) - added by DJPaul 3 years ago.
nav-styles-flyout.patch (3.1 KB) - added by hnla 3 years ago.
Flyout/hover styles for new menus

Download all attachments as: .zip

Change History (33)

comment:1 DJPaul3 years ago

  • Owner set to DJPaul
  • Status changed from new to assigned
  • Type changed from defect to enhancement

comment:2 djpaul3 years ago

(In [3325]) First pass of menu support for BP-Default. See #2691.

comment:3 DJPaul3 years ago

CSS used from the twentyten theme; props WordPress.org team

comment:4 hnla3 years ago

For the fallback_cb to bp_dtheme_main_nav why do we have the depth parameter for wp_list_pages set to depth=1 i.e parent / top level only?

If set to depth=0
All Pages and sub-pages are displayed (no depth restriction)

Is that not preferable? users adding child pages will have them then correctly nested as ul childs of the parent, themers can display those nested elements as they wish ( BP default theme displays as dropdown /flyout links)

Also apropos the new wp nav menus these do not handle child pages correctly or at least in the same fashion as wp_list_pages you cannot get nested ul rendering, only top level elements are generated. There is a depth parameter for nav menus but not sure what that is meant to be affecting?

hnla3 years ago

minor adjustment to wp_list_pages depth Parem from 1 to 0

comment:5 DJPaul3 years ago

The reason why depth=1 is that is what BP-Default currently does, and it was a copy-and-paste (see line 79). No problems changing if it the same styling from the new menus code can be used.

comment:6 hnla3 years ago

I guessed it was a copy over from previous, was just wondering why it was set that way. Setting it to '0' all depths should have no huge issue and should mean that users can simply drop in new child pages of existing ones and have nice automagic dropdowns - as long as wp_list_pages is used in preference to new menus :( , styling is largely already picked up - needs refinement though nested ul receives class token 'children' so is easy to style on the descendant selector or token.

comment:7 hnla3 years ago

Added wp_list_pages to sfhover jquery in global.js

Provides hover support for IE6 as per the adminbar - applicable to the wp_list_pages nav items only.

hnla3 years ago

add #nav li items to existing sfhover jquery for adminbar

comment:8 modemlooper3 years ago

There is no home tab added for custom menus on install now. Also if you add a custom link tab it doesn't get assigned a current item class. Dumbing down, I can bet people are going to ask how to create a home tab. Should this be hard coded?

comment:9 modemlooper3 years ago

/* Menu


ul#nav li.selected:hover > a,
ul#nav li.current_page_item:hover > a,
ul#nav li.current_menu_item:hover > a {

background: #333;
color: #fff;

}

#nav ul {

box-shadow: 0px 3px 3px rgba(0,0,0,0.2);
-moz-box-shadow: 0px 3px 3px rgba(0,0,0,0.2);
-webkit-box-shadow: 0px 3px 3px rgba(0,0,0,0.2);
display: none;
position: absolute;
top: 30px;
float: left;
width: 180px;
z-index: 99999;

}

#nav ul li {

min-width: 180px;
position: relative;

}

#nav ul a {

background: #333;
padding: 10px;
width: 160px;
height: auto;

}

#nav ul li a {

-moz-border-radius-topleft: 0px;
-webkit-border-top-left-radius: 0px;
-moz-border-radius-topright: 0px;
-webkit-border-top-right-radius: 0px;

}

#nav ul.sub-menu li a {

background: #333;
color: #fff;

}

#nav li.current_page_item a:hover,
#nav li.current_menu_item a:hover,
#nav ul.sub-menu li a:hover {

background: #222;

}

#nav li:hover > a,
#nav ul ul :hover > a {

background: #333;
color: #fff;

}

#nav li:hover > ul {

display: block;

}

#nav ul ul {

position: absolute;
top: 0;
left: 100%;
width: 100%;

}

DJPaul3 years ago

comment:10 DJPaul3 years ago

I've tweaked modemlooper's patch a little so things are right-aligned and expand to the left. I also copied in a bit more of the styling from the BP admin bar, and removed the duplicated style section for the original menus.

An issue is that at 3rd level of nested menus, the menus end up opening horizontally flat.
Also, these styles do not apply to the regular menus (i.e. when you do not use a WP 3.0 custom menu); we need to investigate how we are going to add support for that without duplicating all the code.

comment:11 paulhastings03 years ago

  • Summary changed from menu support to [patch] Default Theme - menu support

comment:12 johnjamesjacoby3 years ago

  • Keywords has-patch needs-testing added
  • Summary changed from [patch] Default Theme - menu support to Default Theme - menu support

@paulhastings - Rather than editing the title to include [patch] can you start using the 'has-patch' and 'needs-patch' keywords? [patch] isn't descriptive enough for us to know what the potential status of these tickets (and email notifications) are.

Thanks Paul! :D

comment:13 DJPaul3 years ago

To recap, the outstanding item for this ticket is improved CSS for functionality and styling. hnla is working on an updated patch, any update yet?

comment:14 hnla3 years ago

@DJPaul being worked on as I write. Had hoped to bump into you on the dev chat earlier.

Issue of the flyout positioning is still a thorny one, despite what was remarked on regarding TwentyTen in the last dev chat it doesn't actually do anything specific with the handling of nested levels and actually suffers from exactly the same issue - more and more it feels like an issue that actually is going to have to be managed by the end user i.e if you want to have five levels of child pages on the extreme right hand link then your out of luck and will need to move that link to the left or have fewer levels.

There will be a patch here to play with hopefully latest tomorrow.

hnla3 years ago

Flyout/hover styles for new menus

comment:15 hnla3 years ago

Patch added to test.

Issue of nested levels remains but as above no clear solution one is damned if one does etc.

Based on fact that TwentyTen does suffer from the same issue and that to restrict anything is fundamentally wrong it is felt that this is a user dilemma to resolve in that the user has a choice where to place the top level parent and would realise best option would be to move to left hand position.

Sub ul widths have been reduced using auto & min-width values to minimise screen real estate usage to lessen the onset of the off viewport issue.

Five levels are catered for, and given that ordinarily one would probably advise someone wanting to nest deeper than this ( in fact even before this sort of level were reached) that they perhaps might want to re-think their folder structure in a static site, this ought to be sufficient, however further levels can quite easily be accommodated.

Colours and backgrounds are of course changeable with those added to provide test for parent level highlighting and hover backgrounds, also rules to allow a chisel border effect between sub ul elements is in place but can be removed as sees fit.

Just had thought that the rtl styles will also need to be considered or do they?

comment:16 DJPaul3 years ago

I'm about to put in a patch that adds the CSS required for the drop-down menus as well as styles them. Big thanks to hnla for his previous patches on this which I used as a starting point; I've used a slightly different approach than in the nav-styles-flyout.patch as I felt that didn't scale nicely beyond five levels of menus deep (an edge case, admittedly). Patch will also add the required RTL styles.

Regarding where the nav menus open; they open from the right of the screen, inwards. I don't like the idea that people couldn't use nested menu options on the right-most item just because they are at the edge of the screen. I also don't like the menus opening in this way, but moving the nav to the left of the screen is something I do not want to do without consulting the other core devs and theme authors. We can make a new ticket for that.

comment:17 djpaul3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [3587]) Adds updated CSS styles for the main nav menu. Fixes #2691, huge props to hnla for his initial patches!

comment:18 hnla3 years ago

Paul the issue of the off screen items is simply now shifted the other way!
I don't like the idea that people couldn't use nested menu options on the right-most item

My test case is set up with a variety of nested pages to different depths on various top level nav links, my about page defaulted to being second from left, it has 5 levels nested items these now are lost off the left hand edge of the viewport!

We needed to discuss this really before committing, my reference to being damned if we do damned if we don't was in reference to the fact that there is no real solution, I further go on to make the point that to a degree the site admin/owner must make a few decisions *gasp* on where they position links.

I was intending on adding a second patch to limit the depth on the menus - despite my stated reservations to following that course of action - As you say 'edge case' and I mention structuring any site to have nested greater than 5 deep starts to become bad site design.

I spent a while ensuring that the menus did actually function - albeit with semi reduced frills - in IE6 hence the z-index you probably moved due to puzzling why I had moved it in first place, there was also a very deliberate avoidance of the use of the child selector other than for cosmetic and unimportant decoration; sadly now the dropdowns are simply broken badly, I can see absolutely no virtue in that (It's not a valid argument that BP doesn't support IE6 - I would agree that it makes no effort for a full experience but basic functionality should work) However I agree that this is probably a moot point and I won't raise the issue of IE6 further.

There is a reason that one can't actually use right:-9999em; in the way it has been. Using this property with this value in this manner requires that there is a contrary screen direction. You might find it simpler to simply go with the display:none; route and suffer the likes of me that will spit fury over it's use and issues it causes :)

There are some further additions that I curious about, attempting to apply various positioning aspects, some of these have issues, the obvious one being line wraps - create a link title longer than the width.

Given that the reasoning behind shifting from right flyout to left is a flawed one and that we cannot ever know or predict how users arrange their menu items it is probably best to move back to right flyout, also another consideration is that left flyout like form element layout could be argued to fall under HCI precepts; left flyout is fundamentally awkward for the brain to track unless one has reversed screen direction for rtl human readers.

I added styling hooks (for someone to improve on) keeping the alpha transparency of the original background graphic that existed but changing background on hover and text colour and linking that back up the nodes highlighting the antecedent elements, thought that was reasonably effective albeit requiring perhaps better colour choices?

comment:19 DJPaul3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for the feedback, hnla. Let's work through these issues.

comment:20 djpaul3 years ago

(In [3606]) Nested nav menus will now expand from left-to-right. See #2691

comment:21 hnla3 years ago

@DJPaul

Need to look at various issues arisen from the rewrite of my initial patch.

The most pressing issue is that menus do work to some degree in IE6- to that effect these changes need to be made:

line 217 #nav > li {

needs to become:

#nav li {

new rule directly below:

#nav li li {margin-left: 0;}

Now at least we have correctly displayed top level li items

The z-index I moved for a precise reason, as it is, changed to the li items in various places, should not be necessary for these elements, however what is necessary for IE6 to be able to display the dropdowns is that the z-index be described for a much higher antecedent element e.g #header unless that upsets any other aspects of the layout ( it shouldn't in theory and in practice a cursory glance suggests it doesn't) i would add that property to #header.

This still leaves the issue that the use of the child combinator prevents IE6 displaying all but the first level dropdown, there is no particular fix for this other than the method I used originally but setting a depth of nested levels to an insane number that should never be used for reasons of bad site architecture - there are other possible workarounds, namely a specific Conditional stylesheet for IE6 but that cannot be properly enqueued due to requiring the CC markup wrapping the link tag, I wrote a simple function for a WP install to spit out the CC markup and link tag to wp_head but not 100% happy that is a good approach here? The other approach could be to include a smallish script to fix this and other issues IE6/7 has - it's a classic bit of work that was used quite extensively:
http://code.google.com/p/ie7-js/ Dean Edwards IE7.js

If that was used I would suggest perhaps a 'Content Negotiation' (mime type) script be used to check for browsers that couldn't accept 'application/xhtml+xml' and only those browsers would trigger the add_action() (all modern browsers have been able to handle the xhtml+xml mime type except IE6 and earlier.

comment:22 djpaul3 years ago

(In [3678]) Improve IE6 compatibility with main nav CSS. Props hnla, see #2691

comment:23 djpaul3 years ago

(In [3679]) Update RTL stylesheet for IE6 main nav CSS changes in r3678 (see #2691)

comment:24 hnla3 years ago

removing the z-index from line 287 and moving it to #header line 140

Appears to be fine, most browsers of the modern flavour are stacking the elements in the correct flow regardless where position absolute elements are concerned so do not need z-index necessarily but IE mucks up position absolute so adding z-index to #header sorts that out and all other browsers should be fine with that.

That gives us one level for IE6 and we need to decide on the best approach to solving display of further levels or not as seen fit?

comment:25 djpaul3 years ago

(In [3680]) Improve IE6 compatibility with main nav dropdown menu CSS. Props hnla, see #2691

comment:26 hnla3 years ago

Further to skype conversation earlier there is an issue regards losing focus after the second link in the first level dropdown in other words the hover is lost and dropdown closes. This is a bug in IE reminiscent of the guillotine bug but not, at the moment the simplest fix for this is to move the:

~line 217: background: url( ../images/60pc_black.png );

to the #nav li ruleset

However a further issue arises from the use of that png graphic which I discussed in another ticket, Not tested but I'm assuming that an extreme flicker is being caused by this graphic (rather than IE caching settings, it's not a hover swap after all) in a nutshell a 1px x 1px graphic causes many issues not the least resource draining for the end users system, 1px graphics should never be used there is no saving in file size to be gained (smallest graphic size will be approx the same for a 1px graphic as a 20px one) it would be much better to drop to a solid background hex colour or use the graphic I re factored and attached to the ticket that I haven't the link to :)

comment:28 hnla3 years ago

Yes that's the one, sorry got distracted or was I simply being lazy :)

comment:29 DJPaul3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Moving the background as suggested doesn't work as '#nav li a' is display: block and has the CSS for the curved borders on it; moving the background to the '#nav li' causes a situation where you can see parts of the background image behind the curved borders -- there is one or two pixels in the very corner which aren't masked. We're not changing the border-radius or such for IE6 benefits.

Closing this ticket as fixed now as we have a working implementation in any browser that matters. Any defects etc to go in new tickets.

Note: See TracTickets for help on using tickets.