Skip to:
Content

Opened 23 months ago

Closed 21 months ago

Last modified 21 months ago

#7210 closed enhancement (fixed)

Group type frontend integration

Reported by: r-a-y Owned by:
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.6.0
Component: Groups Keywords: has-patch
Cc: mercime.one@…, espellcaste@…

Description

We introduced the Group Types API in #6784.

This ticket is to flesh out the API for frontend integration.

Attached patch does the following:

  • Integration into the group "Create" and "Manage > Settings" pages:

https://i.sli.mg/KftxKV.png

  • Group type link integration into group header:

https://i.sli.mg/MLq5Vl.png

  • Group type directory integration:

https://i.sli.mg/GWD1uT.png


Dev notes:

Group type directory - I've set a marker (buddypress()->groups->current_directory_type) in groups_directory_groups_setup() to determine if we're on a valid group directory type. This differs from the marker set for member type directories, which is set in our URI parser (bp_core_set_uri_globals()).

bp_groups_set_group_type() - $group_type parameter now accepts an array, so you can pass multiple group types for saving instead of just one group type.

To-do:

  • Group template hierarchy integration.
  • Admin metabox currently uses a <select> dropdown box, but if a group has multiple types, a super admin cannot select more than one type. Probably refactor this to use a multiple <select> dropdown box.

Attachments (5)

7210.01.patch (18.9 KB) - added by r-a-y 23 months ago.
7210.02.patch (22.9 KB) - added by r-a-y 22 months ago.
7210.admin.patch (6.1 KB) - added by r-a-y 21 months ago.
7210.checked_by_default.patch (2.8 KB) - added by r-a-y 21 months ago.
7210-directory-type-message-markup.patch (760 bytes) - added by hnla 21 months ago.

Download all attachments as: .zip

Change History (46)

@r-a-y
23 months ago

#1 @r-a-y
23 months ago

Registering directory support for Group Types is similar to Member Types:

function my_bp_custom_group_types() {
    bp_groups_register_group_type( 'team', array(
        'labels' => array(
            'name' => 'Teams',
            'singular_name' => 'Team'
        ),
        // New for 2.7 - Change 'teams' to whatever slug you want your group type directory to have.
        'has_directory' => 'teams'
    ) );
}
add_action( 'bp_groups_register_group_types', 'my_bp_custom_group_types' );
Last edited 23 months ago by r-a-y (previous) (diff)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


23 months ago

#3 @mercime
22 months ago

  • Cc mercime.one@… added

#4 @espellcaste
22 months ago

  • Cc espellcaste@… added

#5 @r-a-y
22 months ago

02.patch includes some new parameters to bp_groups_register_group_type():

  • 'has_directory' - see comment:1.
  • 'show_in_create_screen' - if set to true, the group type can be selected on the frontend group creation screen and in the group's "Manage > Settings" page. Default: false.
  • 'show_in_list' - if set to true, the group type will appear in the group type list as rendered by bp_group_type_list() (Screenshot ). This defaults to true if 'show_in_create_screen' is set to true. Default: null.
Last edited 22 months ago by r-a-y (previous) (diff)

@r-a-y
22 months ago

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


22 months ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


22 months ago

#8 @r-a-y
22 months ago

FYI, I'm going to commit this in the next day or so.

Then, I'll do further iterations to the admin metabox and template hierarchy afterwards.

Last edited 21 months ago by r-a-y (previous) (diff)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


22 months ago

#11 @r-a-y
21 months ago

In 11142:

Groups: In bp_groups_set_group_type(), allow an array of group types to be set.

See #7210.

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


21 months ago

#13 @r-a-y
21 months ago

In 11144:

Groups: In bp_groups_register_group_type(), introduce new arguments for frontend integration.

This commit introduces the following $args parameters:

  • 'has_directory'
  • 'show_in_create_screen'
  • 'show_in_list'
  • 'description'

Read the inline documentation for full information. The Group Types codex
article will be updated to outline these parameters.

See #7210.

#14 @r-a-y
21 months ago

In 11145:

Groups: Introduce group type functionality on the Groups Directory page.

If a group type of foo is registered with the has_directory parameter,
you can view a group directory for foo at:
example.com/groups/type/foo/

See #7210.

#15 @r-a-y
21 months ago

In 11146:

Groups: Add group type settings fields and a group type list on single group pages.

  • If group types are registered with the 'show_in_create_screen' parameter, group administrators can set the group type when creating a group or when on a group's "Manage > Settings" page.
  • If group types are registered with the 'show_in_list' parameter, a comma-delimited list of group types can be displayed for a group with the bp_group_type_list() function. By default, this function is used on a group page's header.

See #7210.

#16 @r-a-y
21 months ago

In 11151:

Groups: Add current group type to group directory template hierarchy.

See #7210.

#17 @r-a-y
21 months ago

In 11152:

Correct misuse of escaping functions, part 2.

dcavins and I are eating the same pizza. See r11141.

Anti-props r-a-y.

See #7210.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


21 months ago

#19 @r-a-y
21 months ago

@hnla made a good point in Slack today about adding in a new parameter so a group type can be automatically checked during the group creation process.

This is just a note for me to return to this on the weekend.

#20 @hnla
21 months ago

Just moving my general thoughts from slack to ticket

Allowing for a value to be set to represent a default checked=checked on register is a good idea, let the dev or admin decide which 'type' represents a 'normal' or 'default' group for their install.

Of a lesser note are we concerned that in an existing install of groups if one then sets a new group 'type' then creates or associates a group to that type we now have a group list with a column that shows a group type string(label) for perhaps one group but the rest of the type columns for groups are empty, likewise create steps shows one group type, the possibility of creating a non group type exists but isn't clear?

My feeling is that we do:

Types are registered with bp_groups_register_group_type this action prompts BP to assume a type 'Normal' (terminology is wrong but...)

BP sets a type 'normal' for all groups unless create steps or admin backend has overridden with a new or specific type from the choices (yes this step is problematic as it means running through all groups adding values to the type field so perhaps not a great idea).

A new param is provided to allow for a preset checked=checked to exist for a chosen type.

If this param wasn't set and if BP did set a type 'Normal' the default selection would be this value 'normal' set by BP automagically.

A new param is provided that allows for a descriptive string to be set on create steps for the type checkbox items:
'create_step_types_descrip' => 'Please select the group type from the available options or leave as the default selection set',

That last option is really not massively important but could be nice but how that is generated to screen needs thought so that appropriate markup would be generated so likely a step too much at this stage.

Last edited 21 months ago by hnla (previous) (diff)

#21 @hnla
21 months ago

Another brief point (although not sure if this is simply not having found the better or proper approach to follow) is:

It feels as though the group type selection should be on the initial screen along with the name and descrip and thus part of that initial group save, currently I'm needing try and remove plugin creation steps that might be easier if definitively I could check for group type from the very first stage or at the earliest possible moment in the steps.

@r-a-y
21 months ago

#22 follow-up: @r-a-y
21 months ago

admin.patch handles selecting multiple group types in the Groups admin dashboard, as well as showing multiple types in the Groups list table:

http://i.imgur.com/Dem54jA.png

"Not available on the frontend" is a label that is shown when a group type is registered with 'show_in_create_screen' => false.

At the moment, the patch doesn't cover the "Change group type to" bulk actions select box that @dcavins put together. Could use some feedback from him on how to proceed.


I'm going to put together another patch to handle the default checked types that @hnla and I are discussing since comment:19.

Last edited 21 months ago by r-a-y (previous) (diff)

#23 in reply to: ↑ 22 @dcavins
21 months ago

Replying to r-a-y:

At the moment, the patch doesn't cover the "Change group type to" bulk actions select box that @dcavins put together. Could use some feedback from him on how to proceed.

We can improve this in the next release (similar changes would need to be made to the users list table bulk change tool, too). WP itself has a similar inconsistency between the user list table "Change role" select and the use of checkboxes to set multiple roles on the single user manage screen.

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


21 months ago

#25 @r-a-y
21 months ago

In 11176:

Groups Admin: Better support for multiple group types.

If more than one group type is registered:

  • When editing a group, you can now set multiple group types for that group.
  • When viewing the Groups admin dashboard, all group types for a group are displayed in the "Group Type" column.

See #7210.

#26 @r-a-y
21 months ago

In 11177:

bp-legacy: Do not show trailing dash for empty group type description.

Also adds a translator note and HTML-escapes the group type description
string.

See #7210.

#27 follow-ups: @r-a-y
21 months ago

@hnla - I've added checked_by_default.patch regarding comment:20. The new parameter is called 'create_screen_checked_by_default' and defaults to false if not set.

The new parameter name sounds a little wordy to me. Any suggestions for a better name?

Let me know what you think and whether or not this solves your problem.


@dcavins - I agree about moving the bulk change action stuff to the next release.

FWIW, WP 4.7 is going to make things easier to set custom bulk actions:
https://make.wordpress.org/core/2016/10/04/custom-bulk-actions/

Perhaps we can make this a 4.7-only feature?

#28 in reply to: ↑ 27 @hnla
21 months ago

Replying to r-a-y:

@hnla - I've added checked_by_default.patch regarding comment:20. The new parameter is called 'create_screen_checked_by_default' and defaults to false if not set.

The new parameter name sounds a little wordy to me. Any suggestions for a better name?

Let me know what you think and whether or not this solves your problem.

Think it's going to be wordy regardless and I don't see a problem with your choice as for alternative suggestions?

'create_screen_is_type_default'
'create_steps_type_selected'
'create_steps_type_checked'
'create_steps_type_default'

Could ring the changes ad nausea, think it's just a necessarily wordy key.

Last edited 21 months ago by hnla (previous) (diff)

#29 @hnla
21 months ago

This maybe not right time to discuss this or simply missed some salient point in terms of a use cases but:

you can now set multiple group types for that group.

I get that 'types' is simply a taxonomy' in same way those are used in WP e.g. 'category'; but what is the use case for being able or needing to set multiple group types to one single group instance? If we say that a group type has a series of distinct properties as set or registered are we then not going to run into issues where we 'use' these types or do we view this simply as the developers problem to deal with and manage add or don't add multiple taxonomies as you would with a post :)

#30 @hnla
21 months ago

Let me know what you think and whether or not this solves your problem.

It tests fine, as to solving problem I think it is a smart option to be able to have set in advance and certainly prevents accidental type selection, so all round a 'good thing'.

The concerns with pre-existing groups not having a 'type' and columns in admin remaining blank are maybe unfounded, it feels wrong but then nothing will break as a result, there may be use cases where it's best that all groups do have a type though but I can't see that for sure at this moment. and we would have to run some form of update over all groups setting to a 'default'.

My pressing concern is that I do think the creation step initial screen must house the Name, Descrip, and type as the three fundamental pieces of info before we create a group_id then checks on the other steps can be made to bail out of custom group steps more easily or adjust subsequent step screens.

Last edited 21 months ago by hnla (previous) (diff)

#31 @hnla
21 months ago

@r-a-y updating templates I noticed that we're wrapping the bp_get_current_group_directory_type_message() markup group type slug in a pair of <strong> elements, if this is a typographical convention i.e we want it emboldened type then I would rather <b> is used and actually with html5 the notion of the older but valid elements i,b are repurposed with new semantic meaning, although strong is too it still carries a notion of emphasis and importance when this is just a sentence read with flat emphasis. b is allowed to be styled or styled other than as 'bold type' so adding a class allows us to establish how we treat an element with a class as given ( for the moment it would and does remain 'bold' )

Patch gives example.

Of course conversely we simply wrap in span with or without a class and leave to pure styling with no explicit semantics.

Last edited 21 months ago by hnla (previous) (diff)

#32 @hnla
21 months ago

@r-a-y same goes for bp_group_type_list The <strong> element might be better served and the cdata as something like a dl list as we're possibly running a few items in a list.

<dl>
 <dt>Group Types:</dt>
 <dd>Group1</dd>
 <dd>group2</dd>
</dl>

They can then be run inline as they render now or run as default dl list styling

#33 @r-a-y
21 months ago

@hnla - Thanks for the feedback.

My pressing concern is that I do think the creation step initial screen must house the Name, Descrip, and type as the three fundamental pieces of info before we create a group_id then checks on the other steps can be made to bail out of custom group steps more easily or adjust subsequent step screens.

I would consider the group privacy option more pressing than the group type. Group privacy is also listed on the second step as well.

Would welcome other feedback here, but I think we need another cycle to re-evaluate all the other group options during the group creation phase.

I noticed that we're wrapping the bp_get_current_group_directory_type_message() markup group type slug in a pair of <strong> elements

For bp_get_current_group_directory_type_message(), current markup was lifted from the member type directory's message:
https://buddypress.trac.wordpress.org/browser/tags/2.6.2/src/bp-members/bp-members-template.php?marks=1969#L1959

So if we change this here, we'd need to change it for member types as well.

bp_group_type_list The <strong> element might be better served and the cdata as something like a dl list as we're possibly running a few items in a list.

The W3C says the following about lists and groups of links here:

The objective of this technique is to create lists of related items using list elements
appropriate for their purposes. The ol element is used when the list is ordered and the
ul element is used when the list is unordered. Definition lists (dl) are used to group
terms with their definitions.

Although the use of this markup can make lists more readable, not all lists need markup.
For instance, sentences that contain comma-separated lists may not need list markup.

I think the last block applies to us here.

#34 follow-up: @hnla
21 months ago

ok :)

#35 in reply to: ↑ 34 @hnla
21 months ago

Replying to hnla:

ok :)

Actually not ok :) I don't really care what legacy templates do any longer they are dead to me however I do want and expect to be able to make a choice on the newer templates and despite the W3C( who haven't always got things spot on ) ) I probably would favour listing but more to the point to get what I may judge to be right I don't want to be doing silly hoop jumping exercises like having to filter functions just to change something so trivial.

Part of what I wanted to progress albeit slowly was this idea of decoupling core from markup so either a function accepts params to set what markup used or core simply passes over the data and the templates generate the markup; I have to see what works best for the nuoveau templates in this and probably like minded template tags in core so not suggesting anything for the moment but will try and patch a solution for 2.8.

#36 follow-up: @r-a-y
21 months ago

I don't really care what legacy templates do any longer they are dead to me

Are you referring to the group type directory message or the group type list function? I don't mind if we make the change to the directory type message, although the change is quite trivial.

FWIW, <strong> vs. <b> - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/strong#Bold_vs._Strong

strong is a logical state, and bold is a physical state.

Logical states separate presentation from the content, and by doing so allow for it to be
expressed in many different ways. Perhaps instead of rendering some text as bold you want
to render it red, or a different size, or underlined, or whatever.

It makes more sense to change the presentational properties of strong than it does bold.
This is because bold is a physical state; there is no separation of presentation and content,
and making bold do anything other than bold text would be confusing and illogical.

<strong> sounds like the correct use here, but I could be wrong.


On the other hand, bp_group_type_list() is new, so we can make whatever changes that we need to. Right now, the function was created to output a comma-delimited list (I guess the function name isn't named appropriately :) ).

List items are not really appropriate for comma-delimited lists unless you want to use CSS :before and :after declarations, which complicates things.

I guess it all depends on what you want to do with the function. If you plan on using this function for list items, then the function needs to be tweaked to accommodate it.

#37 in reply to: ↑ 27 @dcavins
21 months ago

Replying to r-a-y:

@hnla - I've added checked_by_default.patch regarding comment:20. The new parameter is called 'create_screen_checked_by_default' and defaults to false if not set.

The new parameter name sounds a little wordy to me. Any suggestions for a better name?

I think the parameter name is OK as create_screen_checked_by_default. You could shorten it to create_screen_checked and the meaning would still be pretty clear.

Re bulk changes, I'm hoping to add a multiple edit interface to the groups list like WP uses on the posts list, where you choose several items then click edit and get the quick attribute editor. Would be good for types, status, etc. Could also be used to add members to multiple groups.

#38 @r-a-y
21 months ago

In 11182:

Groups: Add 'create_screen_checked' argument to bp_groups_register_group_type().

If $show_in_create_screen is true, whether we should have our group type
checkbox checked by default during the group creation process. Handy if you
want to imply that the group type should be enforced, but decision
ultimately lies with the group creator.

Props hnla, dcavins.

See #7210.

#39 in reply to: ↑ 36 @hnla
21 months ago

Replying to r-a-y:

I don't really care what legacy templates do any longer they are dead to me

Are you referring to the group type directory message or the group type list function? I don't mind if we make the change to the directory type message, although the change is quite trivial.

FWIW, <strong> vs. <b> - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/strong#Bold_vs._Strong


strong is a logical state, and bold is a physical state.

Logical states separate presentation from the content, and by doing so allow for it to be
expressed in many different ways. Perhaps instead of rendering some text as bold you want
to render it red, or a different size, or underlined, or whatever.

It makes more sense to change the presentational properties of strong than it does bold.
This is because bold is a physical state; there is no separation of presentation and content,
and making bold do anything other than bold text would be confusing and illogical.
}}}

<strong> sounds like the correct use here, but I could be wrong.

No matter @r-a-y I'll work out what we need by way of ease of access to these elements so we can change as see fit, I'll try and make a note on Nouveau tickets about it.

fwiw I think that Mozilla ref sounds like gibberish tbh I would not say strong is correct nor do I understand what on earth they are on about but in html5 they attempted to redefine semantics for all these elements so to describe b as physical is not strictly correct, I do love Mozilla & their MDN site but on this I think they need to review their advice :)

Semantics and markup discussions finished please do what you think best I'm just confusing the issue get things in and we'll worry about this sort of detail another time.

#40 @r-a-y
21 months ago

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

Thanks for hashing this out with me, @hnla. I'm all for semantic markup as well :)

I think all the main tidbits are in. Going to close this one!

Group Types codex article has been updated as well:
https://codex.buddypress.org/developer/group-types/

Last edited 21 months ago by r-a-y (previous) (diff)

#41 @r-a-y
21 months ago

In 11186:

Groups: Fix various notices during implementation of group types.

Props danbp.

See #7210. Fixes #7281.

Note: See TracTickets for help on using tickets.