Opened 8 years ago
Closed 8 years ago
#7105 closed enhancement (fixed)
Improve group management members screen
Reported by: | dcavins | Owned by: | dcavins |
---|---|---|---|
Milestone: | 2.7 | Priority: | normal |
Severity: | normal | Version: | 1.1 |
Component: | Groups | Keywords: | needs-refresh |
Cc: | dcavins |
Description (last modified by )
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.
Attachments (9)
Change History (27)
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
8 years ago
#4
@
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.
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
8 years ago
#9
@
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 updatinggroups/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.
@
8 years ago
Requires #7079 patch. Structural/markup changes. Uses bp_group_has_members() for all three loops.
#10
@
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
@
8 years ago
Cleaned-up version that uses bp_groups_has_members()
for all three loops and adds pagination.
#13
@
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
@
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.
#15
@
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
@
8 years ago
@dcavins heads up that I added a <div>
surrounding the three <div class="widgets">
for ARIA live region in r11126.
List structure changes