Skip to:
Content

BuddyPress.org

Opened 13 years ago

Closed 7 years ago

#3419 closed enhancement (no action required)

bp_core_remove_nav_item() support WP admin bar as well?

Reported by: r-a-y's profile r-a-y Owned by:
Milestone: Priority: normal
Severity: normal Version: 1.5
Component: Core Keywords: dev-feedback has-patch 2nd-opinion close
Cc:

Description

If a BP install is using the WP admin bar (not BuddyBar) and we use bp_core_remove_nav_item() or bp_core_remove_subnav_item(), should these functions remove the appropriate item in the WP admin bar as well?

Attachments (3)

3419.01.patch (1.3 KB) - added by boonebgorges 12 years ago.
3419.02.patch (14.5 KB) - added by imath 8 years ago.
3419.03.patch (42.5 KB) - added by r-a-y 8 years ago.

Download all attachments as: .zip

Change History (22)

#1 @DJPaul
13 years ago

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

The way we're doing it is to use WP's functions to build the WP admin bar, as opposed to putting all the logic and detection into the likes of bp_core_add_nav_item() etc. When the BuddyBar is removed in a future release, we'll backpat the functions to hook up to the admin bar's equivalents.

Admin bar with 1.5 is supported and seems to work just fine, but it was a relatively late addition to 1.5 and so that bit of code hasn't had as much testing. You'll note we don't have an option in the Settings admin screen to let people switch between bars… yet :)

#2 @boonebgorges
13 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 1.5.1
  • Resolution invalid deleted
  • Status changed from closed to reopened

I don't think it makes sense to close this ticket as invalid. r-a-y is right that we should be consistent in our implementation. If a plugin wants to remove, say, the Send Invites subnav from the BuddyBar, it'd be expected that it'd remove it from the WP Admin Bar as well. The fact that we aren't doing this seems like a regression, or at least a way in which the admin bar implementation is incomplete.

If we're not going to make it backward compatible for 1.5 (Paul's right that it's probably too late) then we should do it as soon as possible afterward.

#3 @r-a-y
13 years ago

I've taken a look at this, and it appears to be quite simple to add support.
I'll add a patch in a bit.

#4 @johnjamesjacoby
13 years ago

r-a-y: Still planning a patch?

#5 @r-a-y
13 years ago

What I believed to be "quite simple" turned out to be a complete headache ;)

Not as simple as I thought.

At the end of bp_core_remove_subnav_item(), I had something like this:

	if ( bp_use_wp_admin_bar() ) {
		$bp->temp_slug = $slug;
		add_action( 'wp_before_admin_bar_render', create_function( '', 'global $bp, $wp_admin_bar; $wp_admin_bar->remove_menu( $bp->temp_slug );' ) );
	}

Which works for the very first instance you use bp_core_remove_subnav_item(), but if you use the function more than once, it will use the last instance of it because $bp->temp_slug is overriden each time and because the WP Admin Bar is only rendered at the very end of the page.

Hopefully this gives you superior beings some fodder!

---

Might be easier to tell plugin devs to just use WP's built-in functions for the adminbar:

eg.

function ray_remove_mentions_subnav_item() {
	global $wp_admin_bar;
	$wp_admin_bar->remove_menu( 'mentions' );
}
add_action( 'wp_before_admin_bar_render', 'ray_remove_mentions_subnav_item' );

#6 @boonebgorges
13 years ago

  • Milestone changed from 1.5.1 to 1.6

#7 @boonebgorges
12 years ago

  • Keywords reporter-feedback dev-feedback added; needs-patch removed

3419.01.patch is an idea for how this might be done. It's essentially the same thing that r-a-y suggested, using a stack rather than a single item in the global (to allow for multiple removals).

It's very hackish. To my mind, there is no non-hackish way to do it. The way in which the admin bar is currently set up has nothing to do with bp_core_new_nav_item(). It's set up manually, in each component loader's setup_admin_bar() method. In contrast, the BuddyBar was set up directly out of $bp->bp_options_nav, etc, so there was a guaranteed parallel between the two. IMO, making them separate is a good thing, though perhaps we have made them *too* separate - ideally, they should be linked by default, but it should be possible to unsync them. Anyway, that's kinda beside the point. Because the setup_admin_bar() is more or less hardcoded, there is no reliable way to figure out which WP Admin Bar item corresponds to which BP navigation item. In order for this to be reliable, we need to have some system of automation in place for setup_admin_bar().

Have a look at my patch and see what you think. I am tempted to say that we punt this issue for now, and in the next dev cycle (when we'll really start deprecating the BuddyBar, and so don't have to be as concerned about backpat) we take a fresh look at how our navigation is built, and to what extent it should be tied to the admin bar. Until then, plugin devs should remove their items manually, as r-a-y suggests.

#8 @boonebgorges
12 years ago

Oops, looks like I never uploaded that patch.

#9 @boonebgorges
12 years ago

  • Milestone changed from 1.6 to Future Release

Revisiting this patch demonstrates that it's a sticky wicket indeed. Punting.

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


8 years ago

#11 @imath
8 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 2.7

I think we can actually use the BP Core Nav to achieve this. I've quickly put up a patch for the activity component to sync the BP nav with the WP Admin Nav, which means, removing a subnav item or a nav item to the BP Nav should be taken in account into the WP Admin Nav (i haven't tested yet!)

I think we should make the function i've created BP_Core_Nav::get_admin_nav() a lot more generic, i can see cases where we would like to have a specific primary nav item and its children.

If you think it's a great idea, i'll do the work for the other components ;)

@imath
8 years ago

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


8 years ago

#13 @r-a-y
8 years ago

  • Keywords reporter-feedback removed

@imath - I like 02.patch quite a bit!

I've made some changes to it:

  • 'show_in_admin_bar' now defaults to true.
  • I created a helper method - BP_Component::setup_admin_nav() - so we won't have to duplicate so much code when used across all components.
  • Some components like the Settings component have dynamic nav items that are only applicable when on a profile page. The following admin bar items are still registered manually - Settings > Email, Settings > Delete Account, Groups > Create a Group.

@r-a-y
8 years ago

This ticket was mentioned in Slack in #buddypress by mercime. View the logs.


8 years ago

#15 @r-a-y
8 years ago

  • Milestone changed from 2.7 to Under Consideration

Moving out of the 2.7 milestone.

Will revisit for 2.8.

#16 @DJPaul
7 years ago

When I mentioned this in Slack a couple months ago, it was to prompt discussion if we *want* this feature, it wasn't meant to trigger a lot of sudden dev activity. I'd like to talk in dev chat sometime about this, and we can get a feel whether people think it's a good idea or not.

#17 @DJPaul
7 years ago

  • Keywords 2nd-opinion added

Can I get an opinion, given my last comment above, whether this is something we want, or not -- rather than a technical discussion about the patch. I'm going to close the ticket if we don't get any feedback in a few weeks.

#18 @boonebgorges
7 years ago

  • Keywords close added

A few years removed from the BuddyBar (!), I don't feel that this is such a great idea anymore. On most of the sites I've built, there are structural differences between the items in BP's nav and in the admin bar - by design. The two spaces are for different purposes, and I don't think it makse sense to mirror them. And if we're not going to mirror them 100%, I don't think we should attempt any sort of sync at all.

Adding items in both places is a bit ugly (the APIs are quite different) but not difficult. I don't think it's unreasonable to expect devs to register them separately.

#19 @DJPaul
7 years ago

  • Milestone Under Consideration deleted
  • Resolution set to invalid
  • Status changed from reopened to closed

What @boonebgorges said.

Note: See TracTickets for help on using tickets.