Skip to:
Content

Opened 2 years ago

Closed 6 months ago

#3769 closed defect (bug) (fixed)

WP Admin Bar - Cannot arrange "My Account" items

Reported by: r-a-y Owned by: r-a-y
Milestone: 1.9 Priority: normal
Severity: normal Version: 1.5.1
Component: Core Keywords: has-patch commit
Cc: johnjamesjacoby

Description

If you register a new subnav item under "My Account", you can only position it above the current My Account items or below them.

This is because 'bp_setup_admin_bar' action in the core BP_Component class fires all hooks at the same priority):
http://buddypress.trac.wordpress.org/browser/tags/1.5.1/bp-core/bp-core-component.php#L221

A proposed solution involves removing the "bp_setup_admin_bar" hook from the core BP_Component class and adding it in each component's setup_actions.

Attached is a patch showing how this could be done with the friends component. It's hooked at a priority of 30 as an example.

Attachments (3)

3769.01.patch (1.2 KB) - added by r-a-y 2 years ago.
3769.02.patch (2.3 KB) - added by r-a-y 19 months ago.
3769.02b.patch (6.1 KB) - added by r-a-y 6 months ago.

Download all attachments as: .zip

Change History (15)

r-a-y2 years ago

comment:1 boonebgorges2 years ago

  • Cc johnjamesjacoby added
  • Milestone changed from Awaiting Review to 1.6

I agree that we should be hooking directly to setup_admin_bar. It stinks to force all BP content to be right next to each other. Would like to get feedback from jjj on that, as I know he's big on consolidating BP hooks (which I agree with in general - it's just that in this case, it has limiting factors for UX, which should take precedence).

comment:2 johnjamesjacoby2 years ago

  • Milestone changed from 1.6 to Future Release

r-a-y: I like the idea of your patch, but don't like decoupling this from the BP_Component base class. The goal of that class is to automate all of the things a traditional component does, and this is a step away from that. I'm moving this to Future Release, but let's revisit this again in 1.7.

r-a-y19 months ago

comment:3 r-a-y19 months ago

02.patch adds an additional parameter to BP_Component::start() so we can define an 'adminbar_position' parameter to pass onto BP_Component::setup_actions() and use it there. In the patch, I've used the Friends Component as an example.

I've opted to add a fourth parameter to BP_Component::start() instead of refactoring BP_Component::start() to use a one-parameter array like BP_Activity_Activity::get(), but we can go that route if we decide to.

Let me know what you guys think.

comment:4 DJPaul12 months ago

  • Milestone changed from Future Release to 1.8

Moving this to 1.8 as it has a patch, and probably just needs a refresh and some TLC. Punt to future release if this misses the 1.8 boat.

comment:5 boonebgorges11 months ago

  • Milestone changed from 1.8 to 1.9

comment:6 boonebgorges6 months ago

  • Keywords commit added

I think this technique is fine. My only quibble is with the parameter name 'adminbar_position', which (to my ear at least) suggests positioning on the top-level admin bar, while what's happening here is really positioning on the My Account menu. Maybe something like 'myaccount_menu_order'?

comment:7 r-a-y6 months ago

My only quibble is with the parameter name 'adminbar_position', which (to my ear at least) suggests positioning on the top-level admin bar, while what's happening here is really positioning on the My Account menu. Maybe something like 'myaccount_menu_order'?

I think 'myaccount_menu_order' is an improvement, but that could also refer to the BuddyBar / profile nav menu order. How about 'adminbar_myaccount_order' or 'toolbar_myaccount_order'?

comment:8 boonebgorges6 months ago

How about 'adminbar_myaccount_order'

I like this :)

r-a-y6 months ago

comment:9 r-a-y6 months ago

Before I commit 02b.patch, here's something I just thought of.

The only bad thing about this technique is for plugins wanting to support older BP versions.

This is something I'm currently doing in my custom extended BP_Component class.

	/**
	 * Custom version of the {@link BP_Component::start()} method.
	 *
	 * Since BuddyPress 1.9, start() has four parameters.  However, for backwards-
	 * compatibility with older versions of BuddyPress, we need to pass three.
	 */
	protected function init() {
		$menu_order = 65;

		$rf = new ReflectionMethod( 'BP_Component', 'start' );

		// as of BP 1.9, start() method accepts 4 parameters
		if ( $rf->getNumberOfParameters() == 4 ) {
			parent::start(
				'blah',
				__( 'Blah', 'bp-blah' ),
				constant( 'BP_BLAH_DIR' ) . '/includes',
				array(
					'adminbar_myaccount_order' => (int) $menu_order
				)
			);		

		// older versions of BP
		// this section will probably be removed in a future version
		} else {
			parent::start(
				'blah',
				__( 'Blah', 'bp-blah' ),
				constant( 'BP_BLAH_DIR' ) . '/includes',
			);

			// set properties manually
			$this->adminbar_myaccount_order = (int) $menu_order
		}

	}

init() is used in place of parent::start() in the constructor.

comment:10 boonebgorges6 months ago

There's no particular problem with passing more arguments than needed to BP_Component::start(). You can run the following on BP 1.8:

parent::start(
    'blah',
    'Blah',
    'blah/includes',
    array( 'foo' )
);

and it'll just ignore the additional param.

I guess what you're really pointing to is the adminbar order. But I think it's fine not to support it officially for older versions. It's a new feature. Plugins that want to support it can do a workaround like the one you've suggested here. So, I don't really see an issue.

comment:11 r-a-y6 months ago

There's no particular problem with passing more arguments than needed to BP_Component::start().

For some reason, I thought that a warning would pop up because of the number of arguments being used do not match those in the start() method. Needed the sanity check. Thanks Boone!

comment:12 r-a-y6 months ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 7454:

Add a fourth parameter to the BP_Component::start() method.

This parameter is an array that allows extended classes to define some
properties that need to be set early one for BP_Component to be able
to work its magic.

Currently accepts 'adminbar_myaccount_order', which sets a custom
position for the component menu generated under the WP Toolbar's
"My Account" menu. If this value isn't set, we set the value to 90,
which will generate the menu before the Settings menu is added.

Fixes #3769.

Note: See TracTickets for help on using tickets.