Skip to:
Content

BuddyPress.org

Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#5144 closed defect (bug) (fixed)

Don't set 'Create a Group' button as part of the title

Reported by: rogercoathup Owned by: boonebgorges
Milestone: 2.0 Priority: normal
Severity: normal Version: 1.7
Component: Templates Keywords: has-patch
Cc: karmatosed, mercijavier@…

Description

The "create a group" button shouldn't be hardcoded in core files as part of the Groups directory header.

It makes it near impossible for developers to place it anywhere else (not to mention that's it's also not a semantically correct place to put the button - it's not part of the title).

There also doesn't appear to be any way to filter it out.

Offending code is in bp-groups/bp-groups-screens.php -- in public function create_dummy_post()

Attachments (2)

5144.01.patch (1.6 KB) - added by r-a-y 7 years ago.
5144.02.patch (3.7 KB) - added by boonebgorges 7 years ago.

Download all attachments as: .zip

Change History (32)

#1 @boonebgorges
7 years ago

  • Cc karmatosed added

I agree that this is bad on a number of levels.

The reason it was originally built this way is this. When we moved to theme compatibility in 1.7, we were limited in the areas of the page that BuddyPress could modify. Content itself had to appear in the area controlled by the_content(). And we were able to modify page titles via filter. Because we were bringing over the templates from bp-default, which had the "Create a Group" button right next to the page title, and because we didn't want to make drastic visual changes for 1.7, we did the only thing we could do to put a button in that location - add it to the page title.

I think we should *not* do this in a new set of templates. I'm adding karmatosed as a watcher, so she'll be aware of this issue. However, we can't well remove it from bp-legacy, because many people will already have adjusted for the fact that the button appears there.

I'm leaving this ticket in Awaiting Review for the moment, so that others have a chance to chime in. But I think that in the end it can probably be marked wontfix (for bp-legacy), with the understanding that the new template pack will not have this bug.

#2 @DJPaul
7 years ago

Agree with Boone.

#3 @r-a-y
7 years ago

However, we can't well remove it from bp-legacy, because many people will already have adjusted for the fact that the button appears there.

This is not a template issue. It's a theme-compat issue since the "Create a Group" button is not hardcoded into any templates, but in the groups theme-compat code.

I think, at the very least, we can add a toggle filter to the "Create a ..." buttons so theme devs can remove the button with one-line. See attached patch. (Please excuse the poorly-named filter!)

But I think that in the end it can probably be marked wontfix (for bp-legacy), with the understanding that the new template pack will not have this bug.

Regarding new template pack, the same issue was brought up by karmatosed on GitHub:
https://github.com/karmatosed/buddypress-templatepack/issues/16

Last edited 7 years ago by r-a-y (previous) (diff)

@r-a-y
7 years ago

#4 @r-a-y
7 years ago

  • Component changed from Core to Theme
  • Keywords has-patch added
  • Version set to 1.7

#5 @hnla
7 years ago

The only thing I don't like about the approach is putting the onus on new themes to change something we already agree isn't correct- whatever the reasons for doing it that way at the time.

We're saying first new theme has to add filter false, then has to deal with adding the button code themselves somewhere. Ideally one would run the button code based on if 'bp-legacy' so default condition was not inject into a heading tag where it has no context, in functions.php simply add the button code as variable that can be placed where fancied.

#6 @boonebgorges
7 years ago

hnla - In the new template pack, I assume that the create buttons will be created in a more sane way. The question is what to do about bp-legacy. It's annoying that the "default condition" is to "inject into a heading tag", but this is the way it currently works (and the way the templates have worked since bp-default), so changing it at this point has the potential to break the layouts of many sites that have embraced (or worked around) the current setup. Changing the default behavior will mean changing the markup on many existing sites, in a significant way. We should try not to do this.

r-a-y's patch seems sensible to me - it allows savvy developers and theme designers to opt out of our bad habit, but keeps the default the same for the lifecycle of bp-legacy.

#7 @johnnymestizo
7 years ago

Having the word Group hardcoded into the button is bad too. My install has replaced the group slug with product

#8 @boonebgorges
7 years ago

johnnymestizo - The phrase 'Create a Group' is localizable (it's run through __()). So you should be able to translate it in the same way that you're swapping out the word 'group' everywhere else in your install. See http://codex.buddypress.org/getting-started/customizing/customizing-labels-messages-and-urls/

#9 @mercime
7 years ago

  • Cc mercijavier@… added

#10 @DJPaul
7 years ago

  • Milestone changed from Awaiting Review to 2.0

Moving to 2.0 for attention with the new templates.

#11 @hnla
7 years ago

Can r-a-y patch be committed, I'm re-factoring the template pack buttons using this apply filters approach to remove the buttons from $title but having to hack local core files to get things working and committing my changes up to temp pack repo shortly.

#12 @boonebgorges
7 years ago

My only concern is that the filter defaults to true. If we're looking to phase out the behavior, we should default to *false*, and then filter it to true in bp-legacy. The only problem with this approach would be if someone had built a totally separate templatepack - their title-buttons would disappear with the next BP update. I would guess that hardly anyone has done this, and it's an easy fix if anyone has. Other thoughts?

#13 @hnla
7 years ago

Well then lets default it to false, less concerned with an edge case where someone might have overloaded bp-legacy functions.php. We can change it to 'false' and add a add_filter 'true' to bp-legacy functions.php to preserve that behaviour and in the rare circs where someone has copied over functions.php all they need to add is one line to get things back working. All in all small price for better code standards, and means I can remove the add_filter from temp pack functions.php and the rest of the code is good.

#14 @hnla
7 years ago

On the other hand do we want to pull that markup completely? Probably the ultimate desire, remove markup from a core file? In which case if we do that and leave a filter in place then we simply take a view that functions.php handles the markup for these buttons moving forward; in bp-legacy we add the button markup but in this instance we filter it back to $title preserving that established behaviour for new templates we pass markup to a do_action as I've set things up in template pack - the edge case stands, doubt whether many have overloaded functions.php but if they have it's a simple fix and we do have historical precedence for making little changes like this; iirc user account action buttons.

#15 @hnla
7 years ago

Don't want to present this as a patch (as more food for thought) but but this approach works locally for me:

In bp-groups-screen.php I've refactored directory_dummy_post() $title to remove the if user can create groups and associated markup altogether & adding a new filter:

$title = __( 'Groups', 'buddypress' ) . apply_filters('bp_group_dir_title', $title);

In bp-legacy functions.php I would add:

function group_create_button($title) {

 if( bp_is_active( 'groups' ) && bp_user_can_create_groups()  ) {
  $title .= '<a class="button group-create" href="' . trailingslashit( bp_get_root_domain() . '/' . bp_get_groups_root_slug() . '/create' ) . '">' . __( 'Create a Group', 'buddypress' ) . '</a>';
 }
 return $title;

}	
	
add_filter('bp_group_dir_title', 'group_create_button');

That would preserve the existing behaviour adding the button into the $title.

We now run the button markup, regardless of whether bp-legacy or new templates, from functions.php; with the addition of new do_actions in templates in a new functions.php we would simply run the above function as:

function group_create_button() {

 if( bp_is_active( 'groups' ) && bp_user_can_create_groups()  ) {
  echo '<a class="button group-create" href="' . trailingslashit( bp_get_root_domain() . '/' . bp_get_groups_root_slug() . '/create' ) . '">' . __( 'Create a Group', 'buddypress' ) . '</a>';
 }

}	
add_action('bp_group_create_button', 'group_create_button' );

#16 @boonebgorges
7 years ago

hnla - I do like the idea of having a template-level function for creating the Create a Group button. We do this for other buttons, and I'd imagine it makes life easier for themers. Off the top of my head, I'd say that the function's internals would be better if they used bp_get_button(), and if the value were returned rather than echoed so that it could be used in a filter.

However, for the actual implementation, I think that the filter approach seems a bit arcane. We can just do something like __( 'Groups', buddypress ) . bp_get_group_create_button() in directory_dummy_post(). And then themers (eg in the new templates) can just call the function directly in the template.

#17 @hnla
7 years ago

Only issues I see here are...

Ordinarily I get the notion of 'get' and 'echo' functions, however in this context where we are running code from a template or site files level do we really need a get function, the web dev has direct access to the function and can do whatever they like with it, or are we concerned that a plugin can go grab the get function, but can't think why a plugin would need to grab a function of this nature which is only a simple link; regardless accepting a plugin might need to and adding a filter into the get function to be safer rather than sorry, the next thing I don't really get is...

__( 'Groups', buddypress ) . bp_get_group_create_button()

If this is done in the core function directory_dummy_post() what prevents, or how do we prevent the button being 'injected' into the heading tag again for new templates. The approach I outlined for the template functions meant that in bp-legacy functions.php we ran the function but using add_filter to force the old behaviour but in new templates we would use an add_action to push to a template file.

I'm hoping I'm missing something obvious, I'm re-factoring along your get & echo functions as I do prefer the notion of templates directly running bp_group_create_button() (do_actions to my mind should be re-usuable points for passing content/functions through to where a do_action 'bp_group_create_button' limits the do_actions purpose)

#18 @boonebgorges
7 years ago

what prevents, or how do we prevent the button being 'injected' into the heading tag again for new templates

You're right, my mistake.

in this context where we are running code from a template or site files level do we really need a get function

Yes. If you use apply_filters(), then you have to return a value from your callback. And in order to append a button to that value, you have to have a _get_ function - an echo function won't work.

See 5144.02.patch for my idea of how this could work. It uses your basic idea, but rearranges the way that filters are laid out so that they make a bit more sense (IMO). I like this technique better than r-a-y's original suggestion that we have a one-off filter for the purpose.

#19 @r-a-y
7 years ago

I like the idea behind 02.patch. If anyone doesn't have any other concerns, I say let's roll with it.

#20 @hnla
7 years ago

Add it! As long as we have access to the button markup which is the only other concern. Legacy is taken care of so that isn't a worry.

#21 @boonebgorges
7 years ago

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

In 7942:

Introduce blog- and group-create button functions, and use to build directory headers in bp-legacy

The new functions bp_get_blog_create_button() and bp_get_group_create_button()
standardize the treatment of these buttons by using bp_get_button(). They also
make life easier for themers, who will not need to build the buttons from
scratch.

In this changeset we modify the way that bp-legacy loads these buttons into the
titles of the directory pages. Previously, it was done directly in BP's theme
compat layer, with the result that all template packs had to follow the
questionable practice of embedding a button inside of a header element. Now,
the directory title is a string, but is filtered; bp-legacy then chooses to
add a button by using the new create-button functions + the new filters.

Fixes #5144

Props hnla, r-a-y

#22 @hnla
7 years ago

One last parting comment - and noted that this ticket is in reference to 'groups' - but we need the same treatment for 'blogs' which I added to my new template pack re-factoring but omitted in reference here.

#23 @boonebgorges
7 years ago

One last parting comment - and noted that this ticket is in reference to 'groups' - but we need the same treatment for 'blogs' which I added to my new template pack re-factoring but omitted in reference here.

One last-last parting comment: read the changeset :) [7942]

#24 @hnla
7 years ago

no flies on you then :) yep didn't think to read the changeset 'oops'

#25 @rogercoathup
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi Boone,

I think you are a missing a filter in your patches. For consistency, and to allow us to modify the button that's generated!, the arguments should be filtered before they are passed to bp_get_button.

e.g. For group new topic button, we filter the arguments:

return bp_get_button( apply_filters( 'bp_get_group_new_topic_button', $button ) );

I suggest your patch should be modified similarly, e.g. line 1890 in groups-template should be amended to:

return bp_get_button( apply_filters( 'bp_get_group_create_button', $button ));

And the same for the blog button of course.

Cheers, Roger

Last edited 7 years ago by rogercoathup (previous) (diff)

#26 @rogercoathup
7 years ago

In the same vein, it would probably be a good idea to add a filter on the args in the call to create a new BP_Button in bp_get_button(). This would allow us to generically filter the args for all our buttons, in addition to the filters that allow us to do it on each individual type of button.

i.e.

line 346 of bp-core-template.php should become:

$button = new BP_Button( apply_filters( 'bp_get_button_args', $args );

Last edited 7 years ago by rogercoathup (previous) (diff)

#27 @boonebgorges
7 years ago

In the same vein, it would probably be a good idea to add a filter on the args in the call to create a new BP_Button in bp_get_button(). This would allow us to generically filter the args for all our buttons, in addition to the filters that allow us to do it on each individual type of button.

If you'd like to pursue this, please open a separate enhancement ticket for discussion.

#28 @boonebgorges
7 years ago

In 7947:

Add filters to bp_get_group_create_button() and bp_get_blog_create_button()

See #5144

Props rogercoathup

#29 @boonebgorges
7 years ago

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

#30 @DJPaul
4 years ago

  • Component changed from Appearance - Template Parts to Templates
Note: See TracTickets for help on using tickets.