groups admin manage members buttons hardcoded to core awkward for theme devs

[hnla]

At present the markup for the group admin manage members screen is predominately hardcoded to bp-groups-template.php line ~801.

The buttons for mods & admins list i.e 'promote to', 'Demote to' are rendered nested in a heading tag.

Rendering the buttons in a heading tag is both odd from a semantic view - it could be strongly argued that a name is not a heading, but also, and the real issue is, it hugely limits what can be done from a presentational aspect.

I need these buttons to not be nested in this element or any element other than the parent 'li' so I can properly style them.

The solution?

Possibly two fold?

I'm hacking /groups/single/admin.php

bp_group_admin_memberlist( true, false, true )

Adding in a further parameter

In bp-groups-template.php I added:

bp_group_admin_memberlist( $admin_list = false, $group = false, $group_buttons_ul = false )

Setting the variable to 'false' to create a default state that doesn't change existing layouts so in bp-group-template I do:

				<h5>

					<?php echo bp_core_get_userlink( $admin->user_id ); ?>
					
					<?php if ( empty( $group_buttons_ul ) ) : ?>
					<span class="small">
						<a class="button confirm admin-demote-to-member" href="<?php bp_group_member_demote_link($admin->user_id) ?>"><?php _e( 'Demote to Member', 'buddypress' ) ?></a>
					</span>
					<?php endif; ?>
				
				</h5>
					
					<?php if ( !empty( $group_buttons_ul ) ) : ?>
					<ul id="manage-mem-<?php echo $admin->user_id ?>-buttons" class="small">
						<li>
							<a class="button confirm admin-demote-to-member" href="<?php bp_group_member_demote_link($admin->user_id) ?>"><?php _e( 'Demote to Member', 'buddypress' ) ?></a>
						</li>
					</ul>
					<?php endif; ?>			
			
			</li>

Ideally I would take this further and add all the buttons admin, mods to a function and simply return the function to the above instead of repeating the markup over again, which is simple? enough to do.

Alternatively

I notice that there is an inconsistency in methods as in admin.php after the admin. mods lists are rendered via the template tag we run a group members loop for members not admin or mod already this loop hardcodes the buttons but this time in the frontend view so so does it not make more sense to quickly pull all the markup from bp-groups-template.php into the frontend (my preference) although the matter of backwards compat is not clear but shouldn't be an issue if done correctly to allow the previous button placement to hold true.

I can patch to the first example if feedback suggests this is worthwhile, if not I'll likely end up having to hack core regardless which I would rather not do.

[boonebgorges]

hnla - Your second option is much better than the first. There is no good reason for this markup to be generated in the PHP. It should be split out into a proper loop.

One of two things will have to be done to make this work.
1) Add additional parameters to bp_group_has_members() that allow querying for different member types. I would suggest a 'type' param, which can take 'members', 'mods', 'admins', or an array thereof. Corresponding adjustments would have to happen down the whole chain (all the way down to the query level) in order to make this work.
2) Use the existing 'include' param on bp_has_members() instead

$admins = groups_get_group_admins( bp_get_group_id() );
if ( bp_has_members( array( 'include' => $admins ) ) ) : 
//loop
endif;

I guess I'd say that (2) is the way to go. For one thing, it would keep this patch entirely inside of groups/single/admin.php, instead of requiring adjustments to the query class. Also, I would eventually like to see bp_group_has_members() and its template tags merged into bp_has_members(), so I don't want to put a bunch of extra code into making it work.

As for backpat - we'll simply leave the old bp_group_admin_memberlist() and bp_group_mod_memberlist() in bp-groups-template.php for now. Custom templates that use this tag will continue to work, though we will urge theme authors to migrate to the new method.

hnla, are you up for patching this?

[boonebgorges]

(ps - leaving as Awaiting Review for the moment. If hnla wants to provide a patch, and we go with the less intrusive option (2) I list above, and if the markup remains the same, then there's no reason why we can't squeeze this into 1.3)

[DJPaul]

Boone's plan sounds good.

[boonebgorges]

Also note that you probably won't be able to feed $admins (as cast above) directly into bp_has_members() - you'll need to convert it to a proper array of admin_ids. But you would probably have figured that out :)

[hnla]

I'll digest all this when have a moment, I'll look at what it takes to create a patch along the lines being outlined and whether I have the skills :) One way or another I'll want a solution and would prefer a fully baked one rather than my half baked one I've implemented in my first approach - which does work as a stop gap.

[hnla]

Have the basics sorted so will be able to return this soon but some aspects are confusing and I could do with a little help clarifying them please.

As things stand presently in admin.php bp_group_admin_memberlist() can pass a boolean true/false back, this is used to test on (!)empty($admin_list) and used to either show the demote/promote buttons or set false will show the admins/mods last activity also it's used to set a class token.

I'm unsure why this flip flop exists what reason would the parameter be changed to show the last activity rather than the buttons (given that only the logged in admin is viewing this screen)

If I need to replicate this behaviour then I need to find some other condition to test on but as this is a hardcoded parameter in a frontend template file I'm uncertain why it was thought necessary or what quite I can test on.

[DJPaul]

It looks like those arguments were something that was used in BP 1.0 & 1.1, not 1.2.

[hnla]

So we can assume it's safe to omit then I guess, all the exact same markup will be replicated now in admin.php but via a members loop but omitting this parameter to allow turning off buttons and instead displaying 'last activity'; custom templates that might exist and using bp_group_admin_memberlist() will continue to function as normal, customised stylesheets styling on markup and classes will still be valid as markup/classes remains unchanged, however now option is there for theme devs to re-factor button markup safely if they so choose.

[boonebgorges]

That sounds right to me.

[hnla]

It's about done then just need to check the slightly confusing class on the ul which was being added based on this $admin_list boolean and which I think actually just needs to sit there hardcoded, and do a little tidy up then I'll upload a patch, hopefully in the morning, for perusal.

[hnla]

Here is the patch to test, thought about moving code blocks around but left it as likely it be wrong so can be adjusted to what thought best.

Haven't time presently to puzzle out the single-line class on the ul which looks to be ensuring a line is placed on the ul bottom if only one entry in a section, this was tested for in groups template using the $admin_list parameter which isn't available now actually come to think about it this wasn't a dynamic variable it was true or false based on user set argument so I'm not sure this single-line token ever did really work as designed, regardless it's one final aspect that may need checking over.

[hnla]

new group loops for manage admin and mods in /groups/single/admin.php

[boonebgorges]

Looks nice, hnla!

As I was looking, I realized that we could pull the admins/mods out of the $groups_template without additional queries. 3271.2.patch does this, and moves that stuff to proper template tags.

Not sure I understand your 'final aspect'. It looks to me that all admins/mods are getting lines underneath, regardless of whether there's more than one.

Anyway, please have a look and see whether my revised patch addresses your concern. Bumping this to the 1.3 release, as it's just about finished.

[hnla]

Going to run the 3271.2 in a minute and take a look.

Yep think I was overly concerned with the class issue I think this was legacy code that wasn't doing anything any longer; however I have been modifying things slightly to run a count over the two arrays and an incrementing comparison in the while loop to get a last array item and add a class to remove the last li bottom border and instead add a thicker ul border-bottom, while that works quite nicely of course can't work out how do the same for the member list yet and just broke me firefox :( trying, darn infinite loops.

[hnla]

Scanned the patch quickly and yes much beter to pull out the array back to the group-template I didn't like it sitting in admin.php felt wrong, not getting the string format at the moment.

One thing this may stop me doing though is running my array count as presently I'm counting the $admin_ids and comparing that to a $i++ running in the while loop to get a ===

[hnla]

Quick observations: that class single-list needs to be removed from the ul elements as it's killing lines for my page and we seem to be generating empty class atts on the li elements

Otherwise it works as expected but I'll want to try and find a way of array counting to get a list-last token.

[hnla]

Further observations

Checking back to a 1.2.8 install and this single-line class token comes into play for, surprise, ul sets that only have one li item yet oddly where I have one admin and no li border-bottom the members list that has 3 members has that single-line token hardcoded? so in effect no listed members are underlined??!! although this appears to be a bit of a balls up it's likely become expected behaviour? and will have to be carried through? or have I got this all wrong; *confused*

[hnla]

For my purposes I am doing this to get an array count and last li item (ignore the 'echo' for testing)

		<?php if ( bp_has_members( '&include='. bp_group_admin_ids() ) ) : ?>
		
		<?php $i = 0; echo $count_admins = count( bp_group_admin_ids('', 'array') ) ; ?>
		
		<ul id="admins-list" class="item-list">
			
			<?php while ( bp_members() ) : bp_the_member(); ?>
			<?php $i++ ?>
			<li <?php if($i === $count_admins): ?>class="list-last" <?php endif; ?>>

This gives me a token I can style on to remove the border-bottom, perhaps for bp-default this ought to be used but run on the ul (difficult as it's outside the while loop) and changed to read 'single-line' to try and preserve whatever may be expected by themes?

[hnla]

I've run the #3271.2 patch on both my 1.3 trunk bp-default and my 1.3 dev install and it feels fine so revised patch does indeed address all the concerns of this ticket and I consider it resolved :)

[boonebgorges]

Thanks for the ramblings :) We don't really do these 'counting' classes in other bp-default member lists, so I'd rather avoid them here as well. :last-child should work for your purposes.

[boonebgorges]

(In [4489]) Moves group admin > members markup out of the templatetag and into the template. Fixes #3271, props hnla