Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#6509 closed enhancement (fixed)

Add BuddyPress Menus to Customizer

Reported by: mercime's profile mercime Owned by: imath's profile imath
Milestone: 2.3.3 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc:

Description

Because menu management has been added to the Customizer https://core.trac.wordpress.org/changeset/32806

Attachments (6)

6509.patch (3.9 KB) - added by imath 10 years ago.
bp-menus.jpg (70.3 KB) - added by mercime 10 years ago.
6509.avoid-notice.patch (1.2 KB) - added by imath 10 years ago.
6509.02.patch (4.1 KB) - added by imath 10 years ago.
6509.03.patch (4.0 KB) - added by imath 10 years ago.
6509.04.patch (3.6 KB) - added by imath 10 years ago.

Download all attachments as: .zip

Change History (34)

#1 @imath
10 years ago

  • Milestone changed from Awaiting Review to 2.4
  • Type changed from defect (bug) to enhancement

+1 As i'll explore the customizer for sites profile photos in #6026, i will look at this one for sure!

#2 @imath
10 years ago

I've just posted a request on WP Trac about it, see WP32708

For sure, we'll need to adapt the bp_setup_nav_menu_item() filter to avoid notice errors (the first part of 6509.patch). The second part of the patch is there to show how we could do if the request is ok with WordPress.

Here's how it could look like :
https://cldup.com/J1qT89stMi.png

@imath
10 years ago

#3 @mercime
10 years ago

@imath Thank you for taking a look at this ticket and your patch, plus two thumbs up for the ticket and patch in core, looks great! I attached a screenshot for 2014 theme :) I'm also watching another ticket in core - to allow the customizer to be made wider.

Last edited 10 years ago by mercime (previous) (diff)

@mercime
10 years ago

#4 @imath
10 years ago

@mercime you're welcome :) Thank you for the screenshot on 2014. Watching WP32708 to see how my request will evolve, and to eventually refresh the patch depending on WordPress decisions.

#5 @imath
10 years ago

Unfortunately, WP32708 has been delayed to a future release :(

I don't think we should absolutely try to be in the customizer by faking a post type and change how we're doing so far to have a dynamic menu based on the current user.

We should only make sure to avoid generating a notice error in the customizer for 4.3, and wait for WP32708 for the rest...
see 6509.avoid-notice.patch

#6 @mercime
10 years ago

@imath Looks like there's still hope for your ticket in core. Let's see if it happens in 4.3 beta 3 :)

#7 @imath
10 years ago

@mercime i'm crossing my fingers :)

#8 @imath
10 years ago

6509.02.patch is updated to latest evolutions on WP32708. You'll need the patch @valendesigns shared on the WordPress core ticket to test it.

#9 @imath
10 years ago

  • Keywords has-patch added

@imath
10 years ago

#10 @valendesigns
10 years ago

  • Keywords needs-refresh added
if ( 0 !== $page || 'bp_nav' !== $obj_type ) {
    return $items;
}

Is this condition really necessary? You have the code below it checking for $obj_name and retuning $items when it's not a BP menu object. It just seems redundant to me.

Also, when setting up the $items array the classes key should have a string value. Otherwise, the output string for classes will have commas in it because the Customizer does not convert the array properly.

So this line:

'classes' => array( 'bp-menu', "bp-{$bp_item->post_excerpt}-nav" )

Becomes

'classes' => "bp-menu bp-{$bp_item->post_excerpt}-nav"

#11 @imath
10 years ago

Thanks for your suggestions @valendesigns

I'll update the patch as soon as the WP ticket will be committed.

I'll include your suggestions except that i'll keep

if ( 0 !== $page  ) {
    return $items;
}

Else it would load the same items again and again as soon as you scroll in the available nav items of the customizer.

#12 @valendesigns
10 years ago

Good point! Although, scroll should only trigger when there are more than 10 items, so I guess it is something to consider on a case by case basis. If you have only a couple menu items though, an additional page load should not be triggered when you scroll. Just an FYI for those listening to the conversation.

#13 @imath
10 years ago

scroll should only trigger when there are more than 10 items

Thanks for your comment, and you are completely right! I completely forgot plugins can add BuddyPress user nav items, so we can have more than 10 items.

Next patch will "paginate" BuddyPress nav items. If i understand well the "load more" in the customizer, it's triggered as soon as the reply is not an empty array. Just tested with 11 pages and it seems to be the case.

@imath
10 years ago

#14 @imath
10 years ago

  • Keywords needs-refresh removed

#15 @imath
10 years ago

Filters have been committed to WP Trunk :)

6509.04.patch is adapted to it.

@mercime could you test it to double check everything is ok ?

@imath
10 years ago

#16 @mercime
10 years ago

@imath tested it on WP/BP trunk plus your patch and it's working well :) There's something wonky going on in IE 8, but more likely that it's not due to BP but the customizer.

#17 @imath
10 years ago

thanks for your tests @mercime, i'll check IE8 from work to be sure it's not a side effect of the patch. Then i think we'll be able to have this in. I'm just wondering if we should wait for 2.4 to have this in or to have it in for 2.3.3 (branch 2.3). I'd like to have @johnjamesjacoby 's opinion about it. My personal opinion is : if 2.3.3 will be there to include some fixes for WP 4.3 (eg: #6465), then maybe we should include it or at least a part of if (the notice thing - first part of the patch) in 2.3.3.

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


10 years ago

#19 @johnjamesjacoby
10 years ago

If we can safely make the 2.3 branch work with WP4.3, that would be ideal. If not, or if it's too new or the implementation needs more work, a notice in 2.3 seems okay, but not great.

I'll try and review the patch this weekend, but I'm sure it's better than what we have if it's broken currently.

#20 @imath
10 years ago

Thanks a lot for your feedback @johnjamesjacoby :)

It's not broken. Setting a BP User navigation using the "old" UI is working great. When using the customizer to build nav menus, we have a notice error and setting the BP User Navigation is just not available yet. Thanks to the 2 filters introduced in WP33366 and the patch it will be possible to set a BP User Navigation items directly into the customizer.

#21 @imath
10 years ago

@mercime just checked IE8, i haven't seen any trouble with the patch applied.

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


10 years ago

#23 @mercime
10 years ago

@imath, not seeing any trouble with IE8 now. Must be the trunk version I downloaded when I tested it. Many improvements made since then. Thank you :)

#24 @imath
10 years ago

  • Milestone changed from 2.4 to 2.3.3

mercime: Great!

As discussed during our latest dev-chat i will commit it in the coming days in branch 2.3, making sure to add a context to strings (_x()) and test there are no side effects on WP < 4.3.

Thanks everyone for your help.

#25 @imath
10 years ago

In 10028:

Add BuddyPress dynamic menus management to the customizer

Since WordPress 4.3 nav menus can be managed inside the customizer. Using two specific filters, we are now making sure BuddyPress dynamic menus can also be managed inside the customizer.

Props mercime, valendesigns, johnjamesjacoby

See #6509 (branch 2.3)

#26 @imath
10 years ago

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

In 10029:

Add BuddyPress dynamic menus management to the customizer

Since WordPress 4.3 nav menus can be managed inside the customizer. Using two specific filters, we are now making sure BuddyPress dynamic menus can also be managed inside the customizer.

Props mercime, valendesigns, johnjamesjacoby

Fixes #6509 (trunk)

#27 @mercime
10 years ago

Thank you imath!

#28 @DJPaul
9 years ago

  • Component changed from API to Core
Note: See TracTickets for help on using tickets.