Skip to:
Content

#4869 closed enhancement (fixed)

BP Theme Compat & BuddyPress Widgets styles

Reported by: imath Owned by: boonebgorges
Milestone: 1.8 Priority: normal
Severity: normal Version: 1.7
Component: Theme Keywords: dev-feedback has-patch commit
Cc:

Description

Hi,

Just noticed widgets in BP Theme Compat have no style.

You'll find an image that illustrates, in twentytwelve, the differences between no style and some style and the small changes i've made (attached in the diff).

Attachments (6)

widgets.png (90.8 KB) - added by imath 14 months ago.
Illustrates the diff
4869.diff (4.3 KB) - added by imath 14 months ago.
4869.01.patch (3.3 KB) - added by r-a-y 13 months ago.
4869.02.patch (1.3 KB) - added by DJPaul 13 months ago.
4869.03.patch (6.5 KB) - added by r-a-y 13 months ago.
4869.03b.patch (11.3 KB) - added by r-a-y 13 months ago.

Download all attachments as: .zip

Change History (38)

imath14 months ago

Illustrates the diff

imath14 months ago

comment:1 follow-up: boonebgorges14 months ago

  • Milestone changed from Awaiting Review to 1.7

Do we currently load buddypress.css on *all* blogs when theme compat is enabled? I'd have thought not.

comment:2 imath14 months ago

Just checked, on multisite legacy's buddypress.css and buddypress.js are also loaded in child blogs.

comment:3 in reply to: ↑ 1 imath14 months ago

Replying to boonebgorges:

Do we currently load buddypress.css on *all* blogs when theme compat is enabled? I'd have thought not.

I complete my last comment : the 2 files are loaded even if the primary blog uses BP Default.

r-a-y13 months ago

comment:4 r-a-y13 months ago

  • Keywords 2nd-opinion removed

Just checked, on multisite legacy's buddypress.css and buddypress.js are also loaded in child blogs.

Confirmed.

In 01.patch, I've done a number of things:

  • Move all theme compat logic away from the BP_Legacy class constructor and into the parent BP_Theme_Compat class under a new method - start().
  • Add multisite and multiblog checks to the start() method
  • Add last check for a BP template (members/members-loop.php) as a final resort
  • Rework conditional logic so we don't have to check for bp-default and the members loop until necessary
  • Use the new parent start() method in the BP_Legacy class constructor.

The decision to move all theme compat logic away from BP Legacy was so future BP template packs can easily use the parent start() method instead of having to add that logic in their class constructor.

The multisite and multiblog checks address imath's problem about BP Legacy's styles being added where they shouldn't.

The members/members-loop.php check was added so theme authors don't necessarily need to add add_theme_support( 'buddypress' ) to their theme, which is great as a lot of theme authors will be late in updating their themes.

Let me know what you think.

Last edited 13 months ago by r-a-y (previous) (diff)

comment:5 boonebgorges13 months ago

At a glance, I mostly like this method much better. Doing the logic in the base BP_Theme_Compat sense makes much, much more sense.

The locate_template() brute-force is somewhat unelegant and inefficient. I worry about running this kind of locate_template() call on every single page load (which is what will happen in cases where theme compat actually ends up running). Is it possible to cache this value somewhere where it'll be more easily accessible? Or am I being overly concerned about a miniscule bit of overhead?

The multisite/multiblog stuff is good, though it undercuts the original point of this ticket, which is that styles need to be loaded where the members/groups widgets are loaded - which is possible on non-root blogs. I guess the correct solution for that particular issue is to put some minimal styles into a separate stylesheet, and then enqueue it from the widget's display method.

comment:6 r-a-y13 months ago

I worry about running this kind of locate_template() call on every single page load (which is what will happen in cases where theme compat actually ends up running).

Yeah, I understand that it's an unnecessary check, but the performance should be miniscule as is_file() / file_exists() calls are cached in the stat cache.

We could add this temporarily and look to remove the locate_template() check in 1.8 when hopefully all theme devs are aware of add_theme_support( 'buddypress' ).

If you think that's inefficient, check out BP_Component::includes() and BP's own bp_locate_template(), which both use a ton of is_file() and file_exists() checks when a file doesn't exist! :)

styles need to be loaded where the members/groups widgets are loaded - which is possible on non-root blogs.

We could add a separate method - enqueue_sitewide_styles() - above the multiblog check and add a sitewide stylesheet that would include the widget styles? Or BP core could handle this much like it does the widget JS.

comment:7 boonebgorges13 months ago

  • Keywords needs-unit-tests added

If you think that's inefficient, check out BP_Component::includes() and BP's own bp_locate_template(), which both use a ton of is_file() and file_exists() checks when a file doesn't exist! :)

Touché :)

Or BP core could handle this much like it does the widget JS.

This seems simpler to me. At least for 1.7, I think chances are pretty low that anyone is going to be creating BP template packs so extensive as to include widget stylesheets. For this release, let's roll imath's suggested styles into standalone stylesheets, and enqueue them from the widgets like we do with their js.

As for r-a-y's patch, I'll try to test it this weekend. I think it's sensible to try to include this in 1.7, because it makes life much easier for people building their own theme packs, and we don't want to encourage people to make them more complicated than necessary. I'll try to build some unit tests for it too, if I get a few minutes.

comment:8 boonebgorges13 months ago

(In [6865]) Adds widget styling to bp-legacy buddypress.css. See #4869. Props imath

comment:9 boonebgorges13 months ago

I've done some thinking about this, and I think that r6865 is a good enough solution for now.

For one thing, it's not trivial to add some styles to a widgets-only stylesheet, and then to load only that on child blogs. That's because the widget inherits at least some of its styles from the main buddypress.css. And duplicating those inherited styles in a widgets-only stylesheet will make it more complicated for themers to make consistent changes across sites. In any case, since buddypress.css is fairly stripped down (it only applies to the #buddypress div, unlike the heftier stylesheet of bp-default), it's not likely to clash with other stylesheets, so there shouldn't be any compatibility issues if we load buddypress.css even when it's not used. And CSS files are so aggressively cached by browsers that it shouldn't matter in terms of performance.

More generally, I don't think there's a huge performance issue with running the theme compat process on blogs other than the root blog. For templates, theme compat only kicks in when bp_locate_template() (or a related function) is called, which it won't be on these child blogs. For CSS, loading the stylesheet is fairly harmless; see the paragraph above. The only real worry is our JS, where there could, in theory, be conflicts with markup in a child theme. In a perfect world, we'd do some rudimentary checks before attaching any of our jquery stuff (like, we'd check for the presence of #buddypress or something), but IMO it's only in a fairly extreme edge case that there'd be a problem, so it's not pressing to fix for 1.7.

Finally, while I think it's fine to run theme compat on non-root blogs, I do like some of r-a-y's other suggestions: namely, (a) moving the bp-default, etc checks into a start() method of the parent class, and (b) introducing the locate_templace( 'members/members-loop' ) brute-force BP compatibility check. Moreover, I think that they are very good things to introduce before 1.7, because it'll set the stage for the best practices in child theming/template-pack development (for instance, we'd have a hard time moving the setup_globals() calls later on, if template packs had already implemented it the old way). So I'm going to go ahead and commit those changes, and close this ticket.

comment:10 boonebgorges13 months ago

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

(In [6866]) Moves some core theme compatibility logic to the BP_Theme_Compat parent class

Since every new BP template pack will need to do the same kinds of checks
before starting the theme compat process (is this a child theme of bp-default,
etc), it makes sense for a parent::start() method to handle the logic, rather
than requiring individual template packs to reproduce it in their child
classes.

In addition, this changeset introduces a fallback check for legacy BuddyPress
themes. For themes that are not child themes of bp-default and also have not
declared current_theme_supports( 'buddypress' ), BP_Theme_Compat::start() now
checks for the existence of members/members-loop.php in the current theme, and
if found, it interprets it as a sign that the current theme does indeed support
BuddyPress, and that theme compatibility should be aborted. Theme authors are
still urged to use current_theme_supports( 'buddypress' ) in their template
packs.

Fixes #4869

Props r-a-y

comment:11 hnla13 months ago

6.11 - Extended Profiles

should be:

6.11 - Widgets

comment:12 boonebgorges13 months ago

(In [6869]) Fixes section header in buddypress.css. See #4869. Props hnla

comment:13 DJPaul13 months ago

  • Keywords has-patch needs-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The avatars in the widgets all have different sizes after these commits; the "big" one is shrunk to 40x40, and the small ones down to 20x20 (both down from 50x50). http://cl.ly/image/1f2t1C1i462k

DJPaul13 months ago

comment:14 DJPaul13 months ago

  • Keywords dev-feedback has-patch added

4869.02.patch​ removes some of r6865.

With patch: http://cl.ly/image/0K3U383e1F3f
Current trunk: http://cl.ly/image/2r0b3K2t2d1C

(themes are twentytwelve, twentythirteen, p2)

Patch is dependant on #4902 and #4903 otherwise you'll see some inconsistency after an AJAX request.

comment:15 DJPaul13 months ago

I'd like to fix the widows in the "active x hours ago" strings with a nbsp before the last word, but should ship 1.7 at some point :)

comment:16 johnjamesjacoby13 months ago

Theme compat nitpicks can be put into 1.7.x releases. I'd rather us not quickly write one function to fix widows in one place; could probably be its own ticket.

comment:17 needle13 months ago

The check for the existence of 'members/members-loop.php' seems to be affecting my approach to the BuddyPress upgrade process.

In summary: I maintain a theme which was made compatible with BP via the BP Template Pack plugin such that all the relevant BP theme files were copied into the theme. I also copied the stylesheet (as it was) into the theme and overrode where necessary. My instructions to people were, therefore, to install BP Template Pack and enable only the BP javascript through it. This all worked fine.

With BP 1.7, I was planning not to declare add_theme_support('buddypress') so that the legacy javascript was auto-loaded - and then I would dequeue the legacy CSS file. Now, however, my theme is assumed to support BuddyPress even though I have opted not to declare that it does.

Could BuddyPress at least register the legacy CSS and javascript with wp_register_style() and wp_register_script() so that theme developers have the option to easily add them if they detect version 1.7?

comment:18 r-a-y13 months ago

With BP 1.7, I was planning not to declare add_theme_support('buddypress') so that the legacy javascript was auto-loaded

add_theme_support( 'buddypress' ) tells BP not to load BuddyPress' theme compatibility layer or any of its CSS and JS. So you do want to add this code snippet.

The development version of BP Template Pack automatically adds add_theme_support( 'buddypress' ) in its codebase so you shouldn't have to worry about this.

Try the bleeding version:
https://github.com/boonebgorges/bp-template-pack/archive/master.zip

And let us know if that addresses your problems.

Edit: To clarify, I mean that the next version of BP Template Pack will automatically add add_theme_support('buddypress'), so you shouldn't have to worry about adding this code snippet to your WP theme.

Last edited 13 months ago by r-a-y (previous) (diff)

comment:19 boonebgorges13 months ago

With BP 1.7, I was planning not to declare add_theme_support('buddypress') so that the legacy javascript was auto-loaded - and then I would dequeue the legacy CSS file. Now, however, my theme is assumed to support BuddyPress even though I have opted not to declare that it does.

This seems a bit overcomplicated. If your theme comes with BP templates, then I assume that you want them to be used. The purpose of add_theme_support( 'buddypress' ); is to tell BP not to do theme compat - that is, to use your bundled templates. So, like r-a-y says, I'd suggest that you declare it.

Could BuddyPress at least register the legacy CSS and javascript with wp_register_style() and wp_register_script() so that theme developers have the option to easily add them if they detect version 1.7?

The problem with that is that the bulk of the bp-legacy stuff is not loaded at all if your theme declares current_theme_supports( 'buddypress' ). So, assuming you follow the advice laid out here by me and r-a-y, doing a separate wp_register_script() (rather than relying on wp_enqueue_script() to do the work, as we do now) wouldn't help much.

That said, if you want to use bp-legacy's JS, you should just register/enqueue it directly. The following should work:

wp_enqueue_script( 'bp-legacy-js', BP_PLUGIN_URL . '/bp-templates/bp-legacy/js/buddypress.js' );

comment:20 needle13 months ago

Thanks r-a-y and boonebgorges - yes, working with the github BP Template Pack fixes my issues. I don't need to declare add_theme_support('buddypress') in my theme and can keep instructions to people as they were. I was thinking that it would be nice to dispense with the need for BP Template Pack, but since existing installs of my theme use it, it makes sense to keep going with it.

That said, if you want to use bp-legacy's JS, you should just register/enqueue it directly.

Because the javascript requires localizing, I've found it's not quite as simple as that, though wp_register_script() wouldn't necessarily help here either. Also, I see that BP Template Pack enqueues 'bp-themes/bp-default/_inc/global.js' rather than 'bp-legacy-js' and this enables "Load More" which appends to #content rather than #buddypress.

Thank you both for your help.

comment:21 boonebgorges13 months ago

  • Milestone changed from 1.7 to 1.8

I'm punting this one to clear out the milestone.

r-a-y13 months ago

comment:22 r-a-y13 months ago

imath did some great work in his original patch; however, in the CSS, we make assumptions that every theme will use a widget class.

That is not always the case. For example, TwentyTen uses the widget-container class instead of widget. Therefore, the widget styles will not apply.

The widget JS will also fail because the JS explicitly depends on the widget class:
https://buddypress.trac.wordpress.org/browser/tags/1.7-rc1/bp-core/js/widget-members.js#L2

In 03.patch, I've wrapped all BP widgets with <div class="widget buddypress">. This is so imath's CSS will still work and most importantly, BP's widget JS is able to pick up the widget filter. I've also tweaked the CSS a tiny bit.

We should look at adding this back into the 1.7 milestone.

comment:23 boonebgorges13 months ago

Thanks, r-a-y. But I don't think that we can do what you've suggested. If we add .widget to an inner div, it could cause all sorts of other issues if the .widget class appears on parent elements (as it will in some themes).

It's unfortunate that the styling and js are busted on themes that don't use the .widget class as we expect, but as far as I can tell, this is not a regression: on *secondary* (ie, non-BP) blogs, this always would have been the case.

I think we do need a solution, but I think it needs more thought, and possibly some modification to the way the JS works. In the meantime, I don't think this is serious enough to hold up the release - we can fix in a point release.

r-a-y13 months ago

comment:24 r-a-y13 months ago

If we add .widget to an inner div, it could cause all sorts of other issues if the .widget class appears on parent elements (as it will in some themes).

True; this was something I was thinking about myself after I posted 03.patch.

03b.patch is an alternative patch and should satisfy all requirements.

  • I've renamed all widget classes to widget-buddypress
  • I've changed all widget JS to also look for widget-buddypress so we can keep compatibility with bp-default themes
  • I've changed bp-legacy's CSS from .widget.buddypress to .widget-buddypress

Tested on bp-default, TwentyTen and TwentyTwelve.

Let me know what you think.


Update: We could also change the widget classname to "widget buddypress". The downside to this is if a theme already uses widget in the class, it'll be duplicated. But W3C doesn't consider duplicate class names as invalid HTML (just checked). I know this is considered bad syntax, but thought I'd throw it out there as another alternative!

Last edited 13 months ago by r-a-y (previous) (diff)

comment:25 r-a-y13 months ago

(In [6903]) Restore legacy widget CSS classes for backwards-compatibility.

r6865 changed all widget CSS classes to 'buddypress'.

However, we have to be considerate to themes that are already using the
older widget CSS classes from BP 1.6 and lower.

This commit brings back these CSS classes. See #4869.

comment:26 r-a-y12 months ago

In 6938:

Restore legacy widget CSS classes for backwards-compatibility.

r6865 changed all widget CSS classes to 'buddypress'.

However, we have to be considerate to themes that are already using the
older widget CSS classes from BP 1.6 and lower.

This commit brings back these CSS classes. See #4869.

comment:27 boonebgorges11 months ago

r-a-y - What's left to do on this ticket?

comment:28 r-a-y11 months ago

What's left to do on this ticket?

We need to address what I mentioned in comment:22.

There are two alternatives that I brought up, which are both mentioned in comment:24:

  • attachment:4869.03b.patch - which changes the widget CSS class from 'buddypress' to 'widget-buddypress'. Negatives to this is changing the CSS class, which might interfere with theme devs that are already using the current classes.
  • Adding an additional 'widget' class to the existing widget CSS classes. This is probably the easiest fix, but could cause duplicate CSS classes, but it passes W3C validation.

comment:29 boonebgorges11 months ago

  • Keywords commit added

Let's go ahead and change the widget class. I'm guessing there are very few users who have used that particular selector to change widget styles in the few months since 1.7 came out. If anything crops up during the beta period, we'll consider other routes.

comment:30 r-a-y11 months ago

Let's go ahead and change the widget class. I'm guessing there are very few users who have used that particular selector to change widget styles in the few months since 1.7 came out.

The problem with attachment:4869.03b.patch is the widget class change will also affect bp-default, which might cause CSS issues on customized bp-default themes. So I'm not sure that is the right way to address this either. Damn backpat!

This leaves adding the additional 'widget' class as the only option unless there is a better idea?

comment:31 boonebgorges11 months ago

OK, let's just go with the additional 'widget' class and see what happens during the beta period.

comment:32 boonebgorges11 months ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from reopened to closed

In 7210:

Adds 'widget' class to all BP widget wrappers

Manually adding the 'widget' class ensures that BP's styles and JS are applied
to widgets in themes that do not natively use the 'widget' class for wrapping
widgets (such as Twenty Ten).

Fixes #4869

Props r-a-y

Note: See TracTickets for help on using tickets.