Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#6513 closed defect (bug) (fixed)

New Filter for Groups Widgets and Members Widgets

Reported by: dunhakdis's profile dunhakdis Owned by: boonebgorges's profile boonebgorges
Milestone: 2.4 Priority: normal
Severity: normal Version:
Component: Core Keywords: good-first-bug has-patch
Cc:

Description

Hello!

I often find it difficult to hide the '|' horizontal line separator every time I develop a new theme or design the buddypress widget area for members and groups. It might be a good idea to add filters to those encoded separators.

Instead of directly encoding '|', why don't we provide filters like

<?php echo apply_filters('bp_members_widget_separator', '|'); ?>

for members and

<?php echo apply_filters('bp_groups_widget_separator', '|'); ?>

for groups.

So that I could add some function to the theme's functions.php to easily change those horizontal separators.

<?php
// This will change the default widgets separator from '|' to '>>' (raquo)
add_filter('bp_groups_widget_separator', 'theme_bp_widgets_separator');
add_filter('bp_members_widget_separator', 'theme_bp_widgets_separator');
function theme_bp_widgets_separator(){
	return '&raquo;';
}
?>

Thanks!

-- Joseph

Attachments (2)

widgets.patch (3.1 KB) - added by dunhakdis 9 years ago.
6513.diff (4.1 KB) - added by pareshradadiya 9 years ago.
patch using the span + filter method

Download all attachments as: .zip

Change History (16)

@dunhakdis
9 years ago

#1 @DJPaul
9 years ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to 2.4

I think we'd probably fix this in another way (probably put the output lines into an array, then use array_join, sort of like how the page <title> is generated, but agree with you that this would be a good improvement.

#2 @DJPaul
9 years ago

  • Keywords needs-patch added; has-patch removed

#3 @DJPaul
9 years ago

Sorry, @dunhakdis - I meant to say Hi, and thanks for opening the ticket. :)

#4 @dunhakdis
9 years ago

No Problem.. Thanks :)

#5 @boonebgorges
9 years ago

Yeah, let's definitely do something like this.

I think we'd probably fix this in another way (probably put the output lines into an array, then use array_join, sort of like how the page <title> is generated, but agree with you that this would be a good improvement.

I don't see the benefit of getting fancy when building the markup. Something like widgets.patch seems OK to me. (Though we should (a) run it through the filter just once and store in a variable $separator, and (b) escape before echoing.)

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


9 years ago

#7 @hnla
9 years ago

A few things to consider here.

While in principle the notion of providing access to modify these characters is undeniably a good thing we are leaping over some far more fundamental issues that should be addressed.

  • The use of a series of inline anchors for what amounts to nav links.
  • Outputting bare naked character data to an aggregating non semantic element, not a good thing, it's hard to target those characters and technically upsets formatting blocks.

Ideally we ought to update this and the other widgets that have these 'sort' links to use a construct that can better manage styling and provide some semantic context, classically a ul construct. To ensure we don't upset existing layouts we would add rulesets to manage this new markup to replicate the look we currently default to i.e. horizontal, with a separator that is now largely a decorative visual aid rather than something we need to add in order to vaguely comply with guidelines when links are used for navigation without any semantic expression of their purpose.

If not updating the links in this fashion then we need - at the least - to wrap the characters in span tags to provide formatting context, ease of styling, and ability to provide attr then we can/could provide aria scope 'role="separator"'

The solution proposed uses a filter to access the output, I'm not clear why we need to use this approach as it's somewhat technical (I don't propose it's not provided but it's additional to...) for general users and where we're talking about a widget which by definition passes args back so why are we not updating the widget control to include a text input or maybe safer a set select dropdown of available options for separator characters to use and which we would output to span markup.

We need now to be tackling aspects such as this as fully and finally as possible and I see this as part of the larger task I would like us to begin to tackle with BP templates in the next release/s.

#8 @boonebgorges
9 years ago

Thanks, hnla. This all sounds sensible. The markup for the widgets is some of the oldest in BuddyPress, and should be addressed in a holistic way at some point.

But let's not lose sight of the fact that all the reporter is asking for is a way to change the separator from a hard-coded | to something else. It seems obvious to me that, whatever the long term goals for the widgets may be, we can definitely do something in the short term. If we think that a filter on a piece of hardcoded text (as we have now) will be maintainable, then let's do it. If you think it'd be better to move the character to a CSS :before { content } rule, then let's do that. But let's please do something. Can you make a suggestion about what to do in the short term?

#9 @hnla
9 years ago

But let's not lose sight of the fact that all the reporter is asking for is a way to change the separator

Absolutely, and believe me my remarks above were made with that in mind, but it's important to note we need to address deeper issues and to try and improve as much as logically possible in each iteration just in case it has to last for longer than we cared for it to.

So, and sorry if this is all longwided, we should for the moment add the filter, based on the fact that we can live with the filter regardless of what other changes may be effected.

As suggested the filter would be stored to a variable lets then output that variable to markup adding a class and aria role, so:

<span class="bp-separator" role="separator"><?php echo esc_html( $separator ); ?></span>

Thus we at least have our characters wrapped in a suitable element and not floating around haplessly, we have a styling hook and an aria control for good measure.

For clarity inlne anchors used in this manner essentially read as a sentence with hyperlinks and thus is why the instruction is made that a separator of some nature is provided in order that the links are broken, really the 'pipe' character is the correct option here for this. This instruction is not a decorative or visual cue though it's an assistive one.

If or when we markup these links in a ul construct the hard character is redundant as the ul construct conveys the semantic information naturally via it's 'li' elements then it's perfectly acceptable to use a separator but it now becomes purely a decorative item, then we should use CSS for it's presentational purpose to provide these either as border-left or as said using pseudo elements

#10 @boonebgorges
9 years ago

This all sounds good to me. Let's get a patch using the span + filter method described by hnla.

#11 @dunhakdis
9 years ago

This is great.. Thank you so much guys! :)

@pareshradadiya
9 years ago

patch using the span + filter method

#12 @pareshradadiya
9 years ago

  • Keywords has-patch added; needs-patch removed

#13 @boonebgorges
9 years ago

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

In 9955:

Improve the flexibility of the separators in the action links of the groups and members widgets.

  • Introduce filters 'bp_groups_widget_separator' and 'bp_members_widget_separator' to allow customization of the default pipe character.
  • Wrap separators in a <span> element, to allow for custom styling and proper ARIA role.

Props dunhakdis, pareshradadiya, hnla.
Fixes #6513.

#14 @DJPaul
8 years ago

  • Component changed from General - UX/UI to Core
Note: See TracTickets for help on using tickets.