Skip to:
Content

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#6509 closed enhancement (fixed)

Add BuddyPress Menus to Customizer

Reported by: mercime Owned by: 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 3 years ago.
bp-menus.jpg (70.3 KB) - added by mercime 3 years ago.
6509.avoid-notice.patch (1.2 KB) - added by imath 3 years ago.
6509.02.patch (4.1 KB) - added by imath 3 years ago.
6509.03.patch (4.0 KB) - added by imath 3 years ago.
6509.04.patch (3.6 KB) - added by imath 3 years ago.

Download all attachments as: .zip

Change History (34)

#1 @imath
3 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
3 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
3 years ago

#3 @mercime
3 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 3 years ago by mercime (previous) (diff)

@mercime
3 years ago

#4 @imath
3 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
3 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
3 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
3 years ago

@mercime i'm crossing my fingers :)

#8 @imath
3 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
3 years ago

  • Keywords has-patch added

@imath
3 years ago

#10 @valendesigns
3 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
3 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
3 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
3 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
3 years ago

#14 @imath
3 years ago

  • Keywords needs-refresh removed

#15 @imath
3 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
3 years ago

#16 @mercime
3 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
3 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.


3 years ago

#19 @johnjamesjacoby
3 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
3 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
3 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.


3 years ago

#23 @mercime
3 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
3 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
3 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
3 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
3 years ago

Thank you imath!

#28 @DJPaul
2 years ago

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