Add <div id="buddypress"> wrapper container dynamically.

[r-a-y]

So in a few template parts of ours, we add the <div id="buddypress"> wrapper container.

I think this shouldn't be necessary; we can do this dynamically on the 'bp_replace_the_content' filter hook when BP's Theme Compatibility API kicks in. This ensures that we add our wrapper only when needed.

What do you think?

[hnla]

I would agree that that makes sense, given we dynamically generate the screens we might as well do so for that element, we do have our nouveau classes to consider though class="<?php bp_nouveau_buddypress_classes(); ?>" so we would need to filter that function instead of outputting directly?

Lets add it dynamically I can't see a reason not to.

[r-a-y]

I'm using bp_nouveau_get_buddypress_classes(), which is the return value version of bp_nouveau_buddypress_classes(), so we should be safe.

Going to merge.

[hnla]

We seem to have one unitended result of this change.

We have lost a class that is set for the object_nav wrapper based on a customizer option query, this is upsetting the vertical user nav layout.

Oddly or perhaps not we use the bp_nouveau_get_buddypress_classes() to not only generate classes for it's specific element but also as a more general function to setup other options:

// Set the global for a later use. $bp_nouveau->{$component}->single_primary_nav_layout = $layout_prefs; }

This is used to check whether the option is true and then set the 'vertical' class in bp_nouveau_single_item_nav_classes() dumping that shows it to be undefined so we seem to be running too early or late?

I'm concerned with how these functions are setup, relying on each other in this way, I'm currently thinking that really the nav class function should just fetch it's own options $ work out what it needs to build.

Thoughts on a re-think on the option checks or do we just ID the break and correct if possible, I'm leaning to the former for simplicities sake or a new function to set: $bp_nouveau->{$component}->single_primary_nav_layout on a hook running at a suitable point?

[hnla]

I'm re-factoring the bp_nouveau_single_item_nav_classes() to directly fetch the cust options for those nav layouts rather than rely on checking if the global is set, tried building a new function to set the global but it wasn't happening, this seems the quickest workaround although less a workaround more an acceptable approach imho.

Would you do me a favour & check it out & change if you think it should be otherwise.

[r-a-y]

@hnla - I'm not sure what the problem is based on what you've written. Can you list some steps for me to duplicate?

Was the problem caused by the changes in this PR? Or was it a problem that occurred before my change? It looks like commit f2026b3 fixed what you noted, but I'm not entirely sure.

I'm also not up-to-speed with Nouveau's internals yet, so bear with me!

[hnla]

@r-a-y You need to have set vertical nav display in the customizer for user or groups single that will in turn cause a class to be set 'vertical' managed by the two functions mentioned above.

The issue seems to be the one line in the class function you updated bp_nouveau_get_buddypress_classes() The problem is or was this line: // Set the global for a later use. $bp_nouveau->{$component}->single_primary_nav_layout = $layout_prefs; }

it seems we call this function now too late now for the subsequent check on that global in the following function bp_nouveau_single_item_nav_classes()

the class never gets set due to that global being empty or not set.

I think this is academic though now, my only worry is that the global might not be set ( I moved the setting into the nav classes function though).

My fix simplifies things, we are simply looking to check what value the user has set so I check that directly now in bp_nouveau_single_item_nav_classes() and all works as expected.

Partly the reasoning behind the fix was in thinking the class function mildly confusing in primarily being to generate classes for the bp-wrapper element yet it set a value for something else altogether, I get the logic & fact that the function naming suggests something generic but think things are a little clearer now? or maybe not :)