Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#19136 closed defect (bug) (fixed)

Secondary items should be a flag for the admin bar

Reported by: nacin's profile nacin Owned by: koopersmith's profile koopersmith
Milestone: 3.3 Priority: normal
Severity: normal Version:
Component: Toolbar Keywords:
Focuses: Cc:

Description

Follow-up to #18197, 18197#comment:64, [19120], and an IRC discussion.

The current setup is clever, but doesn't scale and isn't flexible (for plugins, especially).

Attachments (4)

19136.diff (13.4 KB) - added by koopersmith 12 years ago.
groups.diff (12.4 KB) - added by koopersmith 12 years ago.
groups.2.diff (14.2 KB) - added by koopersmith 12 years ago.
groups.3.diff (15.0 KB) - added by koopersmith 12 years ago.

Download all attachments as: .zip

Change History (17)

#2 @nacin
12 years ago

Also test with 18197#comment:122.

#3 @nacin
12 years ago

A wpdevel post coming out of #18880 depends on this ticket.

#4 @ryan
12 years ago

  • Component changed from Administration to Admin Bar

@koopersmith
12 years ago

#5 @koopersmith
12 years ago

Patch builds secondary menus into the core admin bar class. Late binding allows items to be registered to either the primary or secondary menu through the use of the boolean secondary parameter (in add_menu/add_node).

#6 @koopersmith
12 years ago

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

In [19230]:

Add secondary flag to admin bar. fixes #19136.

#7 @nacin
12 years ago

In [19236]:

Promote secondary admin bar items to primary if there are no primary items at time of render. see #19136, #19221.

@koopersmith
12 years ago

#8 @koopersmith
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There are a bunch of issues with the secondary flag. The main problem is that in the case of a secondary menu, setting a flag on each item is far worse than having one common parent.

A good example use case is BuddyPress — BP adds a top level admin bar menu, and provides plugins with a variable to implement their own submenus. If BP wanted to change its menu to become a secondary component of another menu, it would have to update every submenu item with the secondary flag (instead of just changing the parent). The larger issue here is plugins: every single plugin that added a BuddyPress admin submenu would have to update their items with the secondary flag.

The ideal solution is using groups, which would allow us to split submenus into sections. In the case of the BuddyPress menu, it could be converted to a group and be easily styled. No plugins would have to be updated, as the parent ID would still work.

#9 @azaozz
12 years ago

I was with the impression that the secondary flag would be only for second level menus, i.e. all "top > submenu.secondary" will behave exactly like "top > submenu" except they will be outputted at the bottom and have slightly different styling. All children of submenu.secondary would inherit the styling.

Not sure another UL was needed there at all. Seems to just make things complicated and more fragile.

So the secondary flag would be valid only on the first level of submenus "top > submenu", and ignored on any sub-submenus, etc. So if a plugin decides to move it's menu and adds the flag, all children would inherit the styling, no additional flags needed.

Last edited 12 years ago by azaozz (previous) (diff)

#10 @johnjamesjacoby
12 years ago

Replying to azaozz:

I was with the impression that the secondary flag would be only for second level menus, i.e. all "top > submenu.secondary" will behave exactly like "top > submenu" except they will be outputted at the bottom and have slightly different styling. All children of submenu.secondary would inherit the styling.

This is still how it works with groups3.diff except "levels" are "groups" and there is no upper limit to the number of groupings. Previous to this patch, there were only two "types" of menu stylings, and no way to reliably group them by ID. See below:

Not sure another UL was needed there at all. Seems to just make things complicated and more fragile.

If it's the UL I think you're seeing, it is needed to maintain the styling and parentage associated with groups.

So the secondary flag would be valid only on the first level of submenus "top > submenu", and ignored on any sub-submenus, etc. So if a plugin decides to move it's menu and adds the flag, all children would inherit the styling, no additional flags needed.

The "secondary flag" is a poor way to architect this, and actually perpetuates the problem you mention here. It limits any ability to reliably move groups of items in the future, and is a work-around approach similar to the rabbit-hole of the current main admin-nav.

Implementing "true" groups rather than mimic them (numerically or otherwise) allows us to designate physical menu groups with valid and unique identifiers to *know* that a group of items always contain its children regardless of where it might ever be relocated in the future, up to and including anything that a plugin might choose to add anywhere in the binding process.


After review, this patch is actually considerably smaller than it looks, and touches very few guts of the core API itself.

Last edited 12 years ago by johnjamesjacoby (previous) (diff)

#11 @koopersmith
12 years ago

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

In [19429]:

Admin Bar: Secondary is so passé. Groups are the new black. fixes #19136.

#12 follow-up: @ocean90
12 years ago

It would be nice to be able to write a custom render method, but both methods are declared as private. Could we change it to protected or public again?

#13 in reply to: ↑ 12 @nacin
12 years ago

Replying to ocean90:

It would be nice to be able to write a custom render method, but both methods are declared as private. Could we change it to protected or public again?

Carry this into #19371.

Note: See TracTickets for help on using tickets.