Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#5637 closed enhancement (fixed)

Use bp_activity_set_action() to fill bp-templates activity dropdowns

Reported by: imath Owned by: boonebgorges
Milestone: 2.1 Priority: normal
Severity: normal Version: 2.0
Component: Activity Keywords: has-patch
Cc:

Description

bp_activity_set_action() is used to register the activity actions.

This helps building the drop down types in Activity Administration and registering the callable functions that will dynamically generate the translatable action strings.

But in bp-templates :

  • activity/index.php
  • members/single/activity.php
  • groups/single/activity.php

the filters are directly built in it. I've seen that in 2.0 , to avoid backcompat theme issues the 'Profile Updates' type was introduced using one of the templates hook to add a new activity type into the drop down ('bp_activity_filter_options').

I think we can improve this by using bp_activity_set_action() to also set the different template dropdowns options. This way we would (and plugins) only need to use this function to feed admin and front drop downs.

It would also avoid the different bp_is_active() checks in templates. Finally i think it won't have any impact on theme backcompat.

So i suggest the patch attached to this ticket.

Attachments (5)

5367.diff (14.3 KB) - added by imath 5 years ago.
5367.02.diff (14.2 KB) - added by imath 5 years ago.
5367.03.patch (14.7 KB) - added by boonebgorges 5 years ago.
5367.04.patch (14.9 KB) - added by imath 5 years ago.
5637.05.patch (968 bytes) - added by imath 5 years ago.

Download all attachments as: .zip

Change History (14)

@imath
5 years ago

#1 follow-up: @boonebgorges
5 years ago

  • Keywords 2nd-opinion removed

I like this idea. A couple thoughts:

  1. You are excluding types from the group members screen with a blacklist. This is a bit messy. Maybe 'members' and 'group_members' could be different scopes.
  2. The labels you're using in each component don't seem to match the values currently hardcoded into the templates. They probably should.
  3. Strings should be escaped when building the <option> markup
  4. What's the purpose of casting $label to an array and then doing a foreach() loop on it? Is there ever more than one label?
  5. Looks like we never include a 'new_blog' filter. This is odd. (Probably a subject for a separate enhancement ticket.)

@imath
5 years ago

#2 in reply to: ↑ 1 @imath
5 years ago

Replying to boonebgorges:

Updated the patch to 5367.02.diff

  1. You are excluding types from the group members screen with a blacklist. This is a bit messy. Maybe 'members' and 'group_members' could be different scopes.

That's really better, thanks a lot for advice, i've created the 'members_groups' scope because it's in a member's profile groups tab

  1. The labels you're using in each component don't seem to match the values currently hardcoded into the templates. They probably should.

I've checked again, in previous version i forgot to add a domain, but i don't find the wrong labels :( There are 3 labels, i've created as they were not existing :

Activity comment => __( 'Update replies', 'buddypress' )
New Blog         => __( 'Site creations', 'buddypress' )
Changed Avatar   => __( 'Profile Picture Updates', 'buddypress' )
  1. Strings should be escaped when building the <option> markup

Oops! Corrected this. Thanks.

  1. What's the purpose of casting $label to an array and then doing a foreach() loop on it? Is there ever more than one label?

I think around midnight i like to make my life harder! You're right, i've changed this as anyway i need to loop in actions to get the different labels.

  1. Looks like we never include a 'new_blog' filter. This is odd. (Probably a subject for a separate enhancement ticket.)

Yes i guess so, we would only have to set the label & scopes ;)

#3 @boonebgorges
5 years ago

In 5367.03.patch I make the following changes:

  • Call it 'context' instead of 'scopes'. I think this is clearer.
  • Change the wording of the newly introduced labels
  • Change the context names. 'group' and 'member' seem to be more accurate to me, since they're for use on single group/member pages
  • Instead of passing a context to bp_get_activity_show_filters() in the template, do some logic in that function itself to determine the proper context. This is more similar to what we do for other template functions (eg bp_has_activities()). It also avoids your ugly hack of adding _groups to the end of the context in some cases - all the logic is now centralized, but can be overridden by passing a value for $context.
  • Change the "special case" for friends - your technique was maybe unnecessarily abstract, and would any kinds of duplicates. Maybe we want to do this, but we should talk about it first.
  • Clean up code styling a bit

#4 @imath
5 years ago

It looks really great IMO.

I'm adding 5367.04.patch to use bp_get_groups_slug() instead of 'groups' in the switch cases, because if the groups slug is defined differently using for instance :
define( 'BP_GROUPS_SLUG', 'groupes' );
Then all 'member' context activity type are displayed instead of the 'member_groups' one.

I've also added a filter so that it's easier to disable an option. I know it's possible to filter bp_activity_set_action but i thought it would be simpler to restrict options directly from the template function bp_get_activity_show_filters().

@imath
5 years ago

#5 follow-up: @boonebgorges
5 years ago

Thanks, imath! Looking good. Regarding the extra filter: I agree that the added filter will make it easier to remove items. This makes me think that maybe it should be the *only* filter. Two of them seems like overkill. (Why would you need to filter the markup?) I'm just going to do the filter on the array for now, and if someone requests the filter on the markup later, we can consider it then.

#6 in reply to: ↑ 5 @imath
5 years ago

Replying to boonebgorges:

I'm just going to do the filter on the array for now, and if someone requests the filter on the markup later, we can consider it then.

Totally agree :)

#7 @boonebgorges
5 years ago

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

In 8428:

Introduce bp_activity_show_filters(), and use it for generating dynamic activity filter dropdowns

Activity filter dropdowns were previously hardcoded into templates, with
template-specific hooks for plugins to add their own filter values. This
resulted in an inconsistent and awkward process of maintaining filter lists
between components. The new method introduces new parameters to bp_activity_set_action():
$context (an array of the contexts in which the filter should appear) and
$label (the text that should be used when generating the <option> markup). The
new function bp_activity_show_filter() is then used in the templates to
generate a dynamic list of <option> markup, based on the values registered by
the activity types.

Fixes #5637

Props imath

#8 @imath
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry boonebgorges, i just noticed the use of bp_get_groups_slug() when groups component is not active is causing an error :(
Suggesting 5637.05.patch to avoid it.

@imath
5 years ago

#9 @boonebgorges
5 years ago

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

In 8429:

Check that Groups component is active before using bp_get_groups_slug() in bp_activity_get_show_filters()

Fixes #5637

Props imath

Note: See TracTickets for help on using tickets.