[needs-feedback] BP_Button Class (for all components)

[jeffsayre]

When a logged in user is visiting another member’s page, the “Mention this User” and “Send Private Message” buttons are outputted differently than the “Add Friend” button. The output of the former two are controlled by the member-header.php template file whereas the output of the latter is controlled by a template tag function -- bp_add_friend_button(). Furthermore, only the “Add Friend” button offers any means with which to programmatically control the output.

Instead of hardcoding template output directly within a template file, it should be abstracted into a function that allows developers the flexibility with which to control what is output and how it is outputted.

The attached patch removes the hardcoded output for both the “Mention this User” and “Send Private Message” buttons, placing them into a template tag function instead. The new template tag functions also offer filters that allow developers the opportunity to directly control these two important buttons.

This Trac ticket is necessary for the BuddyPress Privacy Component. Without these changes, there is no way to offer members control over who sees and has access to the “Mention this User” and “Send Private Message” buttons.

As an added benefit to this patch, the outputted URLs of the two expanded template tag functions use the wp_nonce_url() for added security. The bp_add_friend_button() already uses link nonce protection. So should these two new functions.

[jeffsayre]

See ticket ​http://trac.buddypress.org/ticket/2567 for a change to the "Add Friend" button

[jeffsayre]

bp-messages-templatetags.php, bp-activity-templatetags.php, and member-header.php changes

[johnjamesjacoby]

Any suggestions on a way to not remove the HTML from the template, and still allow this to be checked/filtered?

These functions used to be this way a while ago, but were turned into link functions instead so they could be customizable in the template.

The bp_add_friend_button probably needs the same treatment.

[jeffsayre]

The way it stands now, whereas the HTML is of course customizable via direct editing, it is not programmatically accessible. IMO, having the HTML in the template does not provide any additional benefits. With the proposed patch, the HTML is completely customizable by using the add_filter function hook. Hard-coding HTML output reduces, not increases designer and developer flexibility.

Also, the purpose of template tags is to allow people to more easily design custom templates without having to copy or cobble together a lot of excess HTML directly from a template file. As an added benefit, since the filter includes the entire outputted HTML string, including the CSS selectors within the division tags, this will provide designers and developers with better control over the default theme output. Without this patch, to make changes to the CSS or outputted text, you have to hack the core CSS and theme files or create a new child theme. Even with doing that, there still is no way for developers to control output.

[johnjamesjacoby]

I tend to respectfully disagree. :)

When creating custom themes for clients, having these buttons all hardcoded into BuddyPress makes it impossible to create a proper stylesheet to match, or change anchors to buttons, without recreating these functions completely. If the buttons were truly customizable via some kind of link/button generating API where ID's and classes could be passed, that would make more sense to me.

Great measures went into abstracting a great bit of that code out of functions and into the template.

How would you feel about creating a general use template tag that can be passed parameters to check privacy settings against instead? We could extend the 'bp_user_has_access' function to be used through-out the template to allow/disallow functionality?

[jeffsayre]

I respectfully disagree with your respectfully disagreeing!

Actually, the only goal in this Trac ticket is to be able to control programmatically the outputted HTML. As it currently stands, that is only possible with the “Add Friend” button. So, whatever solution that is crafted must allow that flexibility.

I do understand the desirability of making sure that template designers have easy access to the CSS selectors. It would be an easy addition to this currently proposed patch to add an additional filter that passes the CSS selectors separately from the rest of the HTML output.

How would you feel about creating a general use template tag that can be passed parameters to check privacy settings against instead? We could extend the 'bp_user_has_access' function to be used through-out the template to allow/disallow functionality?

As an alternate way of providing button filtering, that is not a bad idea. I’m not sure that a general template tag will work for all instances--unless there are three to five parameters that can get passed. Why do I say this? Take a look at ​this Trac ticket.

Just for the “Add Friend” button, three parameters need to be sent to make it possible for privacy filtering to occur in all of the places that the “Add Friend” button can show up. Two of these parameters are needed exclusively for this button. None of the other buttons require this elaborate of a check.

As long as developers have programmatic access to filter out each of the buttons, the final solution does not matter. Of course, as mentioned above, sufficient parameters must be passed to allow for privacy filtering of the “Add Friend” button without having to go through elaborate programmatic gymnastics.

[johnjamesjacoby]

Okay. I'll include a variation of this in 1.2.6. Will probably include new _button functions rather than repurpose the existing functions. If you could do that, I'll commit it. :)

[jeffsayre]

The only goal of this Trac ticket is to provide some filters that allow the stock, default theme buttons to be filtered out in an easy way. So I am fine with whatever accomplishes that goal!

[hnla]

Some quick observations from a frontend developers perspective working these elements. (Apologies for butting in.)

Working in a file such as members-header.php one of the first things that strikes one and needs dealing with is the markup structure for the item-buttons structure; as the name suggests 'items' it really should be a list construct using a series of divs is semantically null. With the present setup I can easily re-work this which is good!

In an ideal world I would prefer a template function that aggregated any items actioned to 'Item-buttons' and that further allowed me to pass parameters to control the markup structure returned (similar to many WP template functions)

Practically speaking please don't make any changes that involve hardcoded markup structure in core files unless we can override it by passing through parameters from the template tag.

[jeffsayre]

I agree with hnla.

The only reason that I extracted the markup into a core function is to keep consistency with the “Add Friend” button as that is how it is done in that button's function. However, if only the actual outputted HTML was keep in all of these button functions, that would be better. In other words, keep the markup in the template files and use it to wrap the function call.

So, if this was the code for the “Send Private Message” button in the template:

<?php if ( is_user_logged_in() && !bp_is_my_profile() && function_exists( 'bp_send_private_message_link' ) ) : ?>
     <div class="generic-button" id="send-private-message">
          <?php if ( function_exists( 'bp_send_private_message_link' ) ) : ?>
               <?php bp_send_private_message_link() ?>
     </div>
<?php endif; ?>

Then this would be the filterable output in the bp_send_private_message_link() function:

$button = '<a href="' . wp_nonce_url( $bp->loggedin_user->domain . $bp->activity->slug . 
'/?r=' . bp_core_get_username( $bp->displayed_user->user_id, $bp->displayed_user->userdata->user_nicename, 
$bp->displayed_user->userdata->user_login ) ) . '" title="' . __( 'Mention this user in a new public message, this will 
send the user a notification to get their attention.', 'buddypress' ) . '">' . __( 'Mention this User', 'buddypress' );

This should be the way it is done for all three buttons: the “Add Friend” button, the “Mention this User” button, and the “Send Private Message” button. It should also be the way it is handled for any other button that a developer may wish to have control over.

Let me know if you want me to repatch this with the above solution.

[johnjamesjacoby]

If you could repatch would be splendid. I will provide cake.

[jeffsayre]

Splendid! I'll bring the ice cream.

I will submit a new patch this evening when I have a spare moment.

I plan on refactoring all three functions ( “Add Friend”, “Mention this User”, and the “Send Private Message” button functions) using the technique suggested in my previous post.

[johnjamesjacoby]

Cool sounds good! :)

[jeffsayre]

Okay, I'm not going to repatch this tonight. I need to think of another solution. I just remembered the other reason I placed the CSS selectors into the HTML outputted by my new functions. It is to offer total control over each button's output.

If the CSS is kept in the template, then even though you can decide not to output the “Mention this User” or “Send Private Message” text, the div.generic-button selector still draws the button borders. This of course is not desirable. But by putting the selectors into the function, you can simply choose to not output the division CSS and therefore the borders do not get drawn. I suppose I could wrap the template div in an if statement, but I'm not sure if that's the proper way to proceed.

[hnla]

And this is indicative of the issues that arise when the model/controller is having to be overly concerned with the view or presentational layer. I would hazard that the only true approach to this is one where all parameters necessary to generate the markup required are passed back from the frontend template function, but that's easy to say.

I'm happy for class tokens to be controlled primarily from the mid tier the issue arises where we have to generate the division hardcoded as it were, if I need to adjust the markup to say a list construct then I would consider a div to be markup bloat as it's simply unnecessary - however I would be prepared to have to suffer that; I could still present the buttons within an unordered li structure and simply try and ignore the div :)

I also would be happy with an IF statement to control things, is that actually an issue?

[jeffsayre]

Yes, some of these issues would not occur if we were working with a MVC construct that enforced tight separation between presentation and logic. But we need to stay within the WordPress method of coding and designing, so we'll have to be more creative and flexible!

You are correct, hnla, in stating that placing a class within the div would be bloat if a designer decided to position the buttons as list items instead. Of course, that is where child themes come in. It is simple enough to create a new, radical design by overriding any default BP theme and its corresponding CSS.

This last point is exactly where the issue we having been debating in this ticket becomes truly problematic and the reason why I did not repatch this ticket last night. If the CSS is abstracted into a templatetag function, then it becomes pretty useless if a designer changes the CSS in their child theme. The only reason I abstracted the CSS in my first patch was because that is the way it is currently done with the "Add Friend" button. I simply wanted to keep consistency between all three buttons and offer a viable means of privacy filtering.

But, after debating this issue in this ticket, it is clear we need a better solution. So what are our options?

1.) If Statements: They could work. It would then be up to each template designer to make sure that the conditionals remained in their new child template (if they made one). Otherwise, privacy filtering for those buttons would not work anymore for any site using that theme.

2.) Graphical Buttons: Whereas CSS is a great tool for not only positioning elements but also graphical rendering to some extent, I'm not too sure of the desirability of using CSS to draw buttons--at least buttons whose visibility needs to be controlled by individual users via privacy settings. So, another possible solution would be to go back to the 1.1.x way of outputting buttons. Back then, ​they were graphic objects. The CSS was used solely for positioning and not for any of the graphical presentation (i.e. button borders or button shading). However, even with the use of graphical buttons in 1.1.x, the CSS was still abstracted into the function.

Final Assessment
Thinking this through, perhaps the first solution is the best alternative. Using graphical buttons instead of CSS-rendered buttons would work, but it would cause another issue. The CSS would still be outputted even if the buttons where hidden due to privacy filtering. This would cause blank divisions which is just bad form.

[hnla]

I would agree that option one - all things considered - is probably the best compromise, I also think that theme developers should not really have a problem dealing with this approach or at least they shouldn't have.

Thing about CSS Jeff is that it is the ONLY method for dealing with the presentation of markup it's the great and holy grail of 'Separation of content from presentation' and freed us from the horrors of the tabular layout so in a sense this notion of graphical button from css styles are in effect one and the same. In some senses it's a shame that we have to try and tie down the application to a default theme. At risk of repeating what we understand ideally the mid tier should never be concerned with markup or presentation at the most these buttons should be returned at the most basic level which is the markup that is not a choice i.e the anchor element which must exist and with that being tokenised. Tokens I do believe should be part of the core it's a method of representing the basic applications elements I can add further ones if I believe I need them but work to a pre defined set that become familiar to work with. I ramble and fly off tangentially though.

I think your final assessment is pretty much as good as it will get for the moment, and further discourse just hold things up?

[jeffsayre]

I'll repatch tomorrow (Friday).

[jeffsayre]

Okay, attached to this ticket is my repatch. I think it is a decent solution given the circumstances of what this ticket is trying to accomplish. I'm keeping the original patch in this ticket for the time being in case anyone wants to see it. At some point, the old patch should be removed. Note: It is crucial to make sure that the proper patch is applied.

As you will see, in this patch, I've managed to keep most of the original template coding. I've added a conditional test that must be passed in order for the division CSS and contents to be outputted. The privacy filter will pass back false to the calling function if a given button is not to be displayed.

Finally, I've extracted the division CSS from the bp_add_friend_button() but kept the other CSS selectors that are found embedded in the if clause conditional statement within that function. It was just too difficult to extract these selectors into a generic, variable-based single output. Also, I kept the division markup the same as it was in the bp_add_friend_button() because at least one of the selectors is used by a jQuery function, so I did not want to make any changes in the selector output. That is why I used the list() construct--which is PHP 4 and 5 complaint--to pass back the requisite variables to the template.

So, with respect to the division wrappers within the member-header.php template, at least all three buttons are now on equal footing.

[jeffsayre]

bp-messages-templatetags.php, bp-activity-templatetags.php, bp-friends-templatetags.php, and member-header.php changes

[hnla]

Just ran a quick test adding the patch to member-header.php along with activity-templatetags changes and seems fine to me.

Is it worth mentioning the Follow/Following button I have in the 'item-buttons' set? just in case it's in any way an oversight? (can't see any issues mind)

[jeffsayre]

I think this patch, if accepted, should be the way all buttons are handled--in core and 3rd-party plugins like the follow/following plugin.

[hnla]

Just had to adjust Follow plugin as it does an add_action for button to 'bp_profile_header_meta' and if moving the div.item-buttons block around it leaves that follow button orphaned at the bottom of div#item-meta, for my purposes added bp_follow_add_profile_follow_button() to the set in div#item-buttons.

[johnjamesjacoby]

Patch looks good Jeff. Will get this in for 1.2.6 and give feedback if needed.

[johnjamesjacoby]

Started looking at the patch in more detail, and ended up liking the overall solution less and less.

I'm going to back-port a solution I had started for 1.3 a little earlier than I wanted. Spent the greater portion of this weekend getting it fixed up for testing. It will introduce a new BP_Button class and template-tags to handle the creation of component buttons. The default theme will attach these button functions to its do_action handlers in the theme itself. This creates a separation between the buttons and the template, and still allows the buttons to be filtered/changed/adjusted/tweaked, similar to how WordPress handles lists.

In 1.3 I think a more elegant solution will be to have templates include some kind of fields.php file, where commonly used elements and their associated HTML are stored.

Rather large patch due tonight or tomorrow that will need some testing.

[johnjamesjacoby]

Attached patch introduces new BP_Button class and associated template tags. Also fixes a few minor issues with references to friends component in core widgets, white-space, code formatting, etc...

This was originally in the queue for 1.3, but if you all can test this patch for 1.2.6 (specifically Jeff regarding privacy) would be outstanding.

I'm honestly reluctant to put this large of a patch through this late in the 1.2.6 dev cycle, so I'd like to get some feedback before considering this a viable solution or not.

In same ways, it complicates the creation of BuddyPress buttons, but in other ways it makes it more flexible and uniform across the components and templates. There is room to expand this also, with custom call-backs and checks before button creation.

[johnjamesjacoby]

Nevermind. Committed in r3260. Patch is too large and realistically no one will patch the diff, and making changes to that diff would be impossible given its size.

If new issues crop up from r3260, post new tickets; otherwise please continue this discussion here.

Jeff; let me know if this is a more workable solution and lets you grab hold of and filter the data accordingly.

[DJPaul]

Typo fix for bp_button.patch - v1

[DJPaul]

Found a typo, patch attached.

[jeffsayre]

I just began testing John's patch before Paul submitted his repatch. :) Either way, I am still looking over the code to understand what it does. Then I will test to see how it may be used for privacy filtering of the various buttons. So, it will be awhile yet before I have any substantive feedback.

[johnjamesjacoby]

(In [3266]) Fixes #2566 (branch) props Paul Gibbs

[johnjamesjacoby]

(In [3267]) Fixes #2566 (trunk) props Paul Gibbs

[johnjamesjacoby]

Jeff. Since this ticket is primarily yours, you can keep commenting here and I will get notified via email.

[r-a-y]

Took a brief look at the code.

Looks good, but any reason why we're introducing new do_actions in group-header.php + member-header.php when we can use the existing "bp_component_header_meta" do_action?

eg. The following is added to group-header.php:

<?php do_action( 'bp_group_header_actions' ); ?>

When we already have:
<?php do_action( 'bp_group_header_meta' ) ?>

[johnjamesjacoby]

To keep it inline with the members template files, and to not litter buttons in with meta, which could be anything.

Mostly just so there are placement options? Suggestions on improvement?

[r-a-y]

I understand why it was done.

Could use a higher priority for main BP buttons in the meta do_action.

eg.

add_action( 'bp_group_header_meta',     'bp_group_join_button', 1 );

I'm just being picky! ;)