Skip to:
Content

Opened 4 years ago

Closed 3 years ago

Last modified 2 years ago

#5817 closed enhancement (fixed)

Additional styling support for BuddyPress Widgets

Reported by: mercime Owned by:
Milestone: 2.4 Priority: normal
Severity: normal Version: 1.7
Component: Templates Keywords:
Cc: mercijavier@…

Description

Currently, buddypress.css styles BP widgets on the premise that theme authors are registering their sidebars like the WP default themes do, where variables can be replaced with the necessary hooks like so:

register_sidebar( array(
	...
	'before_widget' => '<aside id="%1$s" class="widget %2$s">',
	...
) );

However, there are theme authors which do not follow that procedure when registering sidebars. Therefore, many BP widgets are not styled properly because the ID's and classes required are not inserted in the markup. For example, the Grisaille theme from the WP theme repo registers sidebars as follows:

register_sidebar(
	array(
		...
		'before_widget' => '<div class="sidebaritem">',
		...
	)
) );

Therefore the selector founds in buddypress.css, for example .widget.buddypress div.item-avatar img.avatar, are not styling BP widgets since the widget and buddypress classes are not generated in the Grisaille theme's widget container.

Attached image shows the unstyled widgets of the Grisaille theme. I included the Twenty Eleven because the disc list style was showing up.

Patch will follow.

Attachments (11)

bp-widgets.jpg (141.2 KB) - added by mercime 4 years ago.
5817.patch (5.0 KB) - added by mercime 4 years ago.
bp-widgets-after.jpg (138.9 KB) - added by mercime 4 years ago.
5817-margin-liststyle.patch (759 bytes) - added by mercime 3 years ago.
5817-margin-liststyle-avatars.patch (1.3 KB) - added by mercime 3 years ago.
5817-margins-liststyle-avatars.png (507.5 KB) - added by mercime 3 years ago.
5817.40px.patch (389 bytes) - added by r-a-y 3 years ago.
5817-4a.diff (1.8 KB) - added by mercime 3 years ago.
5817-4b.diff (1.3 KB) - added by mercime 3 years ago.
5817.clear.patch (402 bytes) - added by r-a-y 3 years ago.
Clears the list item. Fixes issues with staggering avatars exhibited in Twenty Fourteen.
5817.02.patch (1.9 KB) - added by r-a-y 3 years ago.

Download all attachments as: .zip

Change History (42)

@mercime
4 years ago

#1 @mercime
4 years ago

  • Cc mercijavier@… added

Attached patch adds a .bp-widget class to three BP widgets which have unordered lists: Groups, Members and Friends widgets to enable styling of those lists with lower specificity than the original styles in buddypress.css.

Image attached shows screenshots of the Twenty Eleven and Grisaille themes after the patch is applied.

@mercime
4 years ago

#2 @mercime
4 years ago

  • Keywords has-patch added

#3 follow-up: @DJPaul
4 years ago

The new .item-options and .avatar-block CSS seems too general if you're trying to get the new CSS to only target widget blocks. Did you mean to add the new bp-widget class to these?

I am also not sure about the name of the bp-widget class; to me, that sounds like something I would expect to see on an overall wrapper element, not on ULs in (not all of) the widgets. The duplicated CSS (for backpat) is understandable but kinda annoying.

For a different suggestion, if the root problem is that before_widget isn't guaranteed to contain class=widget, how about we make sure it is? It would be a straightforward change, and this way, the CSS doesn't need to change.

#4 in reply to: ↑ 3 @mercime
4 years ago

The new .item-options and .avatar-block CSS seems too general if you're trying to get the new CSS to only target widget blocks. Did you mean to add the new bp-widget class to these?

I only targeted the widgets which had no styles using the new bp-widget class: Friends, Groups and Members for a simple but effective fix by removing list style, left margin, and styling content within those three widgets.

Per my check, .avatar-block and .item-options are only used in BP widgets, i.e., not used in any other elements within any BP component, so there are no conflicts.
.avatar-block is used only in the "Who's Online" and "Recently Active" widgets
.item-options is used only in the Friends, Groups, and Members widgets

As for BP Blog Posts widget for multisite installations, there's no issue with having a list style, if any, for the links there.

I am also not sure about the name of the bp-widget class; to me, that sounds like something I would expect to see on an overall wrapper element, not on ULs in (not all of) the widgets. The duplicated CSS (for backpat) is understandable but kinda annoying.

Agreed. Used bp-widget as it's a short but specific class name. Can be changed :)

For a different suggestion, if the root problem is that before_widget isn't guaranteed to contain class=widget, how about we make sure it is? It would be a straightforward change, and this way, the CSS doesn't need to change.

That would be ideal. How do we do it? In addition to class=widget, we also need class=buddypress for styles rendered by following specific selectors like .widget.buddypress div.item-meta or .widget.buddypress ul.item-list img.avatar among others. There are also themes which use 'before_widget' => '', instead.

P.S. This is the second trac ticket I created arising from ticket #5804 :)

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

#5 @DJPaul
4 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 2.2

This is probably too late for 2.1 now, but:

...if the root problem is that before_widget isn't guaranteed to contain class=widget, how about we make sure it is?

Something like this https://wordpress.stackexchange.com/questions/108400/add-class-to-before-widget-for-all-widgets-with-a-dropdown-and-a-counter

#6 @DJPaul
4 years ago

  • Keywords good-first-bug added

#7 @r-a-y
3 years ago

This is similar to an issue I brought up in ticket:4869#comment:22:

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

I'm guessing that BP's widget JS also does not work on the Grisaille theme.

Check out the other comments on that ticket for suggested fixes as well.

#8 @mercime
3 years ago

Thank you DJPaul and r-a-y for your feedback :)

The patch which fixes missing classes needs to address:

  • if there is no 'class' attribute in the widget container, then we need to insert 'class="widget buddypress"' via $before_widget
  • if there is a 'class' attribute but we're not sure if they even added "widget" to the class and/or the required variable %2$s" to output "widget buddypress" classes, then we still need to add the classes "widget buddypress" via $before_widget to ensure that we have those classes in the widget container.

Working on a patch.

In the meantime, we also need to address the styles of the BP lists in widgets even after we add the necessary classes to the widget. Attached are two CSS patches for consideration:

  • 5817-margin-liststyle.patch - addresses only the margins and list styles added by some themes
  • 5817-margin-liststyle-avatars.patch - addresses margins, list styles, and makes all avatars in BP widgets the same size: 40px x 40px. Screenshot with style fixes on this patch is also attached (with the patch I'm working on to add before_widget classes).
Last edited 3 years ago by mercime (previous) (diff)

#9 @r-a-y
3 years ago

In 9341:

bp-legacy: Improve styling of widgets with avatars.

This removes the default bullet lists from widgets with avatars that might
not be styled in certain WP themes. Commit also enlarges widget avatars
when displayed in a list.

Props mercime.

See #5817.

#10 @r-a-y
3 years ago

I've committed mercime's CSS changes as they made sense to include.

The last part about adding the widget buddypress classes into our BP widgets for non-standard WP themes is the hang-up. If we were to explore this idea, DJPaul brought up a potential solution in comment:5.

Though, a part of me thinks that the themes that are doing-it-wrong should fix up their widget sidebar calls.

Are we okay with marking this as fixed for 2.2? If we feel that this needs more, let's explore the widget class injection in 2.3.

@r-a-y
3 years ago

#11 @r-a-y
3 years ago

Per https://buddypress.org/support/topic/bp-2-2-beta-1-login-avatar/, I've uploaded a patch so the login widget avatar is appropriately sized at 40px x 40px.

The problem was the login widget avatar's width was being resized at 40px, but the height was not. Tested on twentytwelve and twentyfourteen.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


3 years ago

#13 @r-a-y
3 years ago

In 9394:

bp-legacy: Resize avatar in login widget to 40px x 40px.

r9341 had the side-effect of resizing the login widget avatar to 40px x
60px. This commit ensures that this avatar is sized proportionally.

See #5817.

#14 @mercime
3 years ago

Patches to make sure that the required classes used in buddypress.css are present in the markup of any theme's sidebar widget.

5817-4a.diff attached

  • Targets BP widgets only
  • Inserts a new div with required classes "widget buddypress" within existing widget container
  • Removes possibility of duplicating .widget padding or margin which might have been added to the original .widget container

5817-4b.diff attached

  • Targets BP widgets only
  • Inserts required classes "widget buddypress" before other classes

Having done the above, I agree with @ray that "that the themes that are doing-it-wrong should fix up their widget sidebar calls". Which is why: 1) a. I also posted a note for the theme author at https://themes.trac.wordpress.org/ticket/11170#comment:3 to consider making the changes in his theme even if the last theme update was 2 years ago; and 2) I did not include any support for themes which did not add the class attribute in their theme's $before_widget

@mercime
3 years ago

@mercime
3 years ago

#15 @mercime
3 years ago

  • Keywords has-patch added; needs-patch removed

#16 @DJPaul
3 years ago

  • Keywords good-first-bug removed
  • Milestone changed from 2.2 to 2.3

Let's do the other changes on this for 2.3.

@r-a-y
3 years ago

Clears the list item. Fixes issues with staggering avatars exhibited in Twenty Fourteen.

#17 @DJPaul
3 years ago

  • Milestone changed from 2.3 to 2.2.1

Ray attached a patch for a bug these 2.2 commits caused in the members widget per https://buddypress.org/support/topic/style-of-buddypress-members-widget-broke-after-updating-to-2-2/

Moving to 2.2.1

#18 @johnjamesjacoby
3 years ago

Patch looks fine to me.

#19 @johnjamesjacoby
3 years ago

  • Keywords commit added

This still looks good. I can commit today if no one beats me to it.

#20 @r-a-y
3 years ago

In 9473:

bp-legacy: Clear list item in widgets with avatars.

Fixes an issue with staggering avatars exhibited in some themes such as
Twenty Fourteen.

See #5817.

#21 @r-a-y
3 years ago

In 9474:

bp-legacy: Clear list item in widgets with avatars.

Fixes an issue with staggering avatars exhibited in some themes such as
Twenty Fourteen.

See #5817 (2.2-branch)

#22 @r-a-y
3 years ago

  • Keywords commit removed
  • Milestone changed from 2.2.1 to 2.3

I've committed the clear list item fix.

The other patches regarding widget CSS injection need to be reviewed and looked at for 2.3.

#23 @r-a-y
3 years ago

  • Milestone changed from 2.3 to 2.4

#24 @mercime
3 years ago

It is evident that we do need to inject the widget and buddypress classes in before_widget to provide the styling support we provide in buddypress.css for our widgets because we cannot rely on themes to provide the necessary elements to inject the classes for the following reasons:

  1. It is not even a requirement in the WordPress Theme Review process to add the class element and the variable in before_widget, i.e., a theme can be approved even without those elements. The WP.org Theme Check on widgets used in the theme submission process doesn't check for said elements.
  1. Since Matt joined in the theme review slack channel, the theme review team has already started researching for the best way for a separate upload process for theme authors to get their themes in the WP theme repository without needing to go through a full review by a WP theme reviewer. That separate process will use the Theme Check plugin, probably beefed up a little but doubtful if it will check for the elements we need.
  1. Just to note that, while not directly related to this discussion, even the WP.com VIP Scanner on widgets does not check for the before_widget items at all, only the register_sidebar location ID.

So at this point in addition to 5817-4b.diff posted above, I suggest removing buddypress widget from the calls to WP_Widget constructor in respective components to remove duplication of classes. Advice on how to proceed with this ticket would be great.

@r-a-y
3 years ago

#25 @r-a-y
3 years ago

  • Type changed from defect (bug) to enhancement
  • Version set to 1.7

mercime - Thanks for the rundown about how Theme Check does not look for dynamic CSS classes.

It may be a good issue to raise here:
https://github.com/Otto42/theme-check

As for widget CSS class injection, I've tweaked your 5817-4b.diff patch and made it so it only adds the 'widget' and 'buddypress' CSS classes if they do not exist.

I tested this on the Grisaille and Twenty Fifteen theme and it works.

Note: I haven't altered any of our current widgets in the patch because we haven't done anything to them since v1.7 and they still work ;)

Yes, I'm aware that for the twenty-* themes, that the 'widget' class is duplicated due to the classname:
https://github.com/WordPress/WordPress/blob/master/wp-content/themes/twentyfifteen/functions.php#L136
https://buddypress.trac.wordpress.org/browser/tags/2.3.2/src/bp-members/bp-members-widgets.php?marks=44,257,407#L41

Last edited 3 years ago by r-a-y (previous) (diff)

#26 @mercime
3 years ago

Thank you @r-a-y. Tested the patch and it works as you mentioned on the Grisaille theme and the WP bundled themes :)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


3 years ago

#28 @r-a-y
3 years ago

  • Keywords commit added

mercime - Let's add this. Since you have sheparded this ticket and since you now have commit access, I'll let you do the honors!

Last edited 3 years ago by r-a-y (previous) (diff)

#29 @mercime
3 years ago

@r-a-y Thank you. I was about to follow up on this one :)

#30 @mercime
3 years ago

  • Keywords has-patch commit removed
  • Resolution set to fixed
  • Status changed from new to closed

Patch has been committed to trunk but erroneously attributed to #6532, my bad. See https://buddypress.trac.wordpress.org/ticket/6532#comment:13

Closing this ticket as fixed.

#31 @DJPaul
2 years ago

  • Component changed from Appearance - Template Parts to Templates
Note: See TracTickets for help on using tickets.