Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

#7105 closed enhancement (fixed)

Improve group management members screen

Reported by: dcavins's profile dcavins Owned by: dcavins's profile dcavins
Milestone: 2.7 Priority: normal
Severity: normal Version: 1.1
Component: Groups Keywords: needs-refresh
Cc: dcavins

Description (last modified by dcavins)

The group management members screen is a bit odd.

The structure of the members lists is unlike any list anywhere else in BP. There are three members loops, one for admins, one for mods, and the members, and the admins and mods loops use bp_has_members() with the include argument while the members loop uses bp_group_has_members().

I'm attaching patches that make the structure more like other lists and employs bp_group_has_members() for all three loops.

See #7093, #7094, #7095 for related enhancements.

Attachments (9)

7105.structure.1.diff (4.0 KB) - added by dcavins 8 years ago.
List structure changes
7105.bp-groups-has-members.1.diff (5.1 KB) - added by dcavins 8 years ago.
Use bp_groups_has_members() for all three loops.
manage-members-before-patch.png (26.6 KB) - added by dcavins 8 years ago.
Manage members list before patch
manage-members-w-patch.png (25.6 KB) - added by dcavins 8 years ago.
Manage members screen after patches.
7105.02-structure-only.patch (6.5 KB) - added by dcavins 8 years ago.
Requires #7079 patch. Structural/markup changes. Does not change the loop function.
7105.03-loops-mod.patch (7.1 KB) - added by dcavins 8 years ago.
Requires #7079 patch. Structural/markup changes. Uses bp_group_has_members() for all three loops.
7105.04.patch (10.4 KB) - added by dcavins 8 years ago.
Cleaned-up version that uses bp_groups_has_members() for all three loops and adds pagination.
Manage_Public_One_BP_Test.png (58.6 KB) - added by dcavins 8 years ago.
Example view in Twenty Thirteen
Manage_Public_One_BP_Test-search.png (43.1 KB) - added by dcavins 8 years ago.
Example of search results and messages.

Download all attachments as: .zip

Change History (27)

@dcavins
8 years ago

List structure changes

@dcavins
8 years ago

Use bp_groups_has_members() for all three loops.

#1 @dcavins
8 years ago

  • Description modified (diff)

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


8 years ago

@dcavins
8 years ago

Manage members list before patch

@dcavins
8 years ago

Manage members screen after patches.

#3 @DJPaul
8 years ago

  • Milestone changed from Awaiting Review to Future Release

#4 @r-a-y
8 years ago

  • Version changed from 2.5.0 to 1.1

Markup changes look fine to me. Would it make sense to mirror the markup of the /members/members-loop.php file here?

The switch to bp_group_has_members(), while it makes logical sense to do so, is actually more query-intensive as that utilizes BP_Group_Members_Query, which has to re-query for the group administrator / group moderator IDs and populate some group member data. Would like @boonebgorges to chime in here.

Also, just to keep in mind that offereins has a patch to split up the admin.php template into separate template parts - #7079. That patch would probably need to be refreshed as well.

#5 @DJPaul
8 years ago

  • Keywords needs-refresh added; has-patch removed

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


8 years ago

#7 @dcavins
8 years ago

  • Owner set to dcavins
  • Status changed from new to accepted

#8 @dcavins
8 years ago

  • Milestone changed from Future Release to 2.7

#9 @dcavins
8 years ago

I’ve refreshed this patch as though #7079 has been committed. I’ve also made the following changes:

  • Change structure of list items to more closely match members/members-loop.php as suggested by @r-a-y. (Should we be also updating groups/single/members.php to be more like that?)
  • Added the action bp_group_manage_members_admin_item to the admin and moderator list items. (Already was applied to the member list.)
  • Added “none found” messages for each loop to support the addition of a search box in #6835.

I thought more about using bp_groups_has_members() instead of bp_has_members() for the admin and mod loops, and, while I'm not assuming we'll go that direction, there are some advantages. If we used bp_groups_has_members() for the loops, we could also then use bp_group_member_joined_since() (which seems useful) and easily add pagination when necessary to the admin and mod loops via bp_group_member_needs_pagination().

I've run some performance comparisons, and with the loops as is, the page in my test environment (a small group) required 73 queries and .0327s of query time. Using bp_groups_has_members() for all three loops required 77 queries and .0266s of query time. I’ll attach both versions of the patch, so others can check it out if they’d like.

Version 0, edited 8 years ago by dcavins (next)

@dcavins
8 years ago

Requires #7079 patch. Structural/markup changes. Does not change the loop function.

@dcavins
8 years ago

Requires #7079 patch. Structural/markup changes. Uses bp_group_has_members() for all three loops.

#10 @boonebgorges
8 years ago

While @r-a-y is correct that there's a performance hit to using bp_group_has_members(), it's definitely the more semantic and consistent thing to do. My vote is to use bp_group_has_members(), and to work separately to eliminate redundant queries from BP_Group_Member_Query. See https://buddypress.trac.wordpress.org/ticket/5407#comment:7 for some prior discussion.

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


8 years ago

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


8 years ago

@dcavins
8 years ago

Cleaned-up version that uses bp_groups_has_members() for all three loops and adds pagination.

@dcavins
8 years ago

Example view in Twenty Thirteen

@dcavins
8 years ago

Example of search results and messages.

#13 @dcavins
8 years ago

I've uploaded a new version of the patch above, 7105.04.patch and a couple of screenshots showing how the changes look. Everyone, but especially @mercime and @hnla: if you have a minute for a glance at the patch or a test drive, I'd appreciate your feedback.

I'm planning to commit this change before 2.7-beta because this is the perfect opportunity to change this template file, and the old screen isn't great for large groups.

One change I'm still considering making is adding an action hook in the "content" area of each member item so that devs could add other useful info to the member list, like extended profile data and such, to help group admins make good decisions.

Thanks for your feedback!

#14 @mercime
8 years ago

@dcavins Tested and worksforme. Checking a11y, new markup structure passes :)

One thing I'd like to see changed are the old demote/promote/etc/ links into buttons per issue I posted in bp-nouveau repo. Users of assistive technologies will be confused that the "promote link" and "demote link" do not go anywhere but do something else instead. But if it's not within this ticket's roadmap, let me know and I'll add it in new ticket re Buttons and Links :)

EDIT: using #members-list ID causes duplicate IDs when the Members widget is active on the same page as this screen. It's the old markup, duplicate IDs also happens on the Members Directory and Group > Members screens.

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

#15 @hnla
8 years ago

Nothing much to add really mercime has it covered so will step back, just be careful with changes on this scale it's one thing to test in our environs and passing as worksforme another to try and extrapolate how those changes might stack up in whatever custom scenarios that could exist - but indeed changes are technically proper the nesting of so much code in a heading element just jaw dropping - commit it!

#16 @mercime
8 years ago

@dcavins heads up that I added a <div> surrounding the three <div class="widgets"> for ARIA live region in r11126.

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


8 years ago

#18 @dcavins
8 years ago

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

In 11138:

Improve group manage members screen.

  • Change structure of list items to more closely match

members/members-loop.php.

  • Use bp_group_has_members() for each loop.
  • Added the action bp_group_manage_members_admin_item to the admin

and moderator list items. (Already was applied to the member list.)

  • Added “none found” messages for each loop to support the addition of

a search box in #6385.

  • Added bp_group_member_joined_since() meta to member list items.
  • Added pagination controls to each list when needed.

Props dcavins, hnla, mercime, r-a-y.

Fixes #7105.

Note: See TracTickets for help on using tickets.