Update BP_buttons class to accept new arg param for $element_type

[hnla]

We need to make the BP_button class a little more flexible and update and increase it's $args parameters while adding a couple of conditional blocks to build either opening/closing tags based on the arg e.g $element_type = 'button'.

Being able to specify in the args to use the button element or default to the existing anchor element wrapping the $content will be helpful in facilitating this ticket and it's requirements to be able to use the button element.
​https://github.com/buddypress/next-template-packs/issues/78

[boonebgorges]

Sure, this seems like a good enhancement to me.

Would it make sense for button to be the default element used by BP_Button? bp-legacy would probably have to continue to use whatever it's currently using, as themes are probably targeting the element type, but it sounds like it might be better moving forward to default to the semantic tag.

[hnla]

Thought it best or even necessary to default to the anchor in order to not disrupt existing sites as 'a' to 'button' is a huge change and easily a breaking one. Think we have to make the change on new templates by passing the param 'element_type' => 'button' or we have to have some means of changing the new default for legacy using templates?.

Trying to fathom the best approach as Nouveau is using a series of custom classes and functions finally running bp_get_button() so trying to see if it's not easier to just lift BP_button and transpose to Nouveau although even if is tackling at function /class core is preferable or should in reality a bit of functionality like this not live in templates period - sorry thinking out loud

[hnla]

First pass rough re-working to allow new args params for buttons.

[hnla]

Rough first pass pushed up, works more or less but not checked in too much detail, just proof of concept.

I see many attendant problems with doing this though at various levels here and on Nouveau.

I think updating this class though to offer the updated param options even if not directly utilized ought to be a good thing!

[hnla]

Adds name and value pairs to the atts - forms need them you know!

[hnla]

-02 adds name and value attr was forgetting that the button element is primarily a form control.

Still a hasty patch not deeply tested!

[hnla]

Updates patch to provide ul construct logic and element rendering.

[hnla]

Final patch adds a third and final provision to build actions using a ul/li construct which really was the original re-configuration I wanted to have this class able to generate.

With A, UL, Button elements available as $arg choices we should cover all bases and makes the class as flexible as it needs to be or should be expected to be (I'm sure there are attr and general approaches that may need adding or changing).

In general these changes check out run under the Nouveau custom wrapper and button classes with minor issues that would need ironing out, so assuming by and large that this also holds true for the more straightforward BP construction of custom buttons.

[hnla]

Correct confusion as to what args 'wrapper' refered to.

[hnla]

04 corrects a point of confusion in passing args around multiple functions & classes 'wrapper' is not the top level wrapper in Nouveau but as we loop it's the looped elements wrapper, thus a ul is wrong and it should be the li element.

[hnla]

Minor adjustments, data attr esc_html.

[boonebgorges]

Thanks for your work on these patches, @hnla. Aside from some code formatting issues (indentation, etc), the approach looks good.

A question about parameter names and element types. You're using button_type, button_name, button_value. Maybe these could be renamed type, name, and value? That way, if we ever decide to allow BP_Button to create elements of type input, we could reuse the same parameters.

In the documentation, you say that button_type accepts 'button' or 'submit'. Should this be enforced? (IMO, no - it's implied that the developer should adhere to HTML spec. We should remove the "accepts" language from the parameter documentation for this reason, and just in case they decide to add more possible values, like 'reset'.)

data_attr is a bit funky, because it asks for fully formed attribute name-value strings. Is it overkill to accept an array and parse it? Eg:

'data_attr' => array(
    'bp-buttons' => 'foo,bar',
    'whatever' => 'baz',
),

which would become data-bp-buttons="foo,bar" data-whatever="baz"? This could be useful elsewhere in BP.

[hnla]

@boonebgorges Yep the naming conventions is a tricky one, not seen so clearly in this one class are the various and slightly longwinded args building that have to originate in the Nouveau calling function and filter down through to bp_get_button().

I think with the button names or 'button' prefix I was wanting to be clear in what I was dealing with, also I think I had decided that there was not going to be a choice to run the element as 'input', too many choices spoil the developer :)

I will re-think that, I will allow for the possibility of an $element = 'input' and re-set those button var names, & hope it doesn't confuse things. We can't justify denying the input use it's legit and our job done well is to facilitate dev options and bp_get_button() then becomes a more useful little generic function.

In the documentation, you say that button_type accepts 'button' or 'submit'. Should this be enforced? (IMO, no - it's implied that the developer should adhere to HTML spec. We should remove the "accepts" language from the parameter documentation for this reason, and just in case they decide to add more possible values, like 'reset'.)

I may need some additional clarity and help on this aspect - whether we are simply referring to documentation or how in actuality the attr is set. I'll review the attr as this is a mandatory one and I'm defaulting to type="button" due to assuming majority of the time these action type buttons will fall outside a form construct, however equally type="reset" is valid if not a little out of fashion but ought to be allowed for.

data_attr is a bit funky

Yep just knew someone would spot that :) sheer laziness in having to split the attr name into two parts and going stir crazy with arg values - I did consider what you describe above in the nested array and will re-factor to that.

I'll make these changes and push a patch up later today.

Last thing I could do with is an eye cast over the final apply_filter() L:356 just to make sure that's correct.

[hnla]

Updates for BP_Button class, new element type, improved data attr parsing and rendering.

[hnla]

-06 Updates for the mentioned improvements in Boone's comments.

• Loop over data_attr, now passed in as a nested array key/values so we can build multiple sets to add to an element.

• Re-factors the button array names to make more generic and re-usuable for input type elements.

• Adds provision to let dev choose input as element and adjusts attr where required.

• Allow for the setting of 'reset' type.

[r-a-y]

@hnla - Can you provide some examples of bp_get_button() with your new parameters?

Some code review:

• The patch looks for li as a specific wrapper and then you set a new property called li_class. If you set wrapper to li, why do you need a li_class when there is an existing wrapper_class?
• I think element should be renamed to link_element to be consistent with existing properties like link_id, link_rel, etc. You're mixing element with these link_* properties on the $this->contents line, which made me infer that you want to use these link_* properties.

BP_Button is starting to get hard to read.

What if we did something like this?

<?php
bp_get_button( array(
        // this replaces 'wrapper'
        'parent_element' => 'div',

        // this replaces all 'wrapper_*' params
        'parent_attr' => array(
                // whatever attributes you want as key / value
                'id' => 'my-id',
                'class' => 'parent-class',
        ),

        // this will be the main button element
        'button_element' => 'a',

        // this replaces all 'link_*' params
        'button_attr' => array(
                'class' => 'generic-button',

                // data-attr could also live here
                'data-attr' => array(
                )
        ),
) );

Of course, we'll add backward compatibility so those older parameters will still work.

[hnla]

A lot of the reasoning is based on having to try and accommodate the changes from the viewpoint of nouveau's custom classes & functions and is where I test and pass through my params.

The patch looks for li as a specific wrapper and then you set a new property called li_class. If you set wrapper to li, why do you need a li_class when there is an existing wrapper_class?

There is method in the madness here and lots of stress in trying to puzzle through this for what's frankly something that just needs to be quick and simple, it may seem odd I agree but those params for the li help the custom functions pass through a bool that allows the main class know what to do., badly explained but... it probably makes more sense if you saw the Nouveau functions but I haven't pushed those up yet.

I'm sure you're right about a lot of this but equally some of it's a little trickier than it appears at first glance, and yes I can see you code example looks& reads better but if you change things it needs to be done in tandam with some custom $buttons args. because it all falls over all too easily :)

I want to avoid confusion and try to get some sort of better standard of semantics into buddypress - button isn't a great choice in general, $element is what I'm trying to change, button was in use but dropped at Boone's suggestion and I added input as an element choice.

You're likely correct that in reality the wrapper_class simply doubles the li_class but before that changes one needs to run through it all carefully, I'll take one last pass through it and look at that, the nested arrays as commonsensical as they might be a step too far for me on this timewise.

[hnla]

Ignore the last patch here '06' everything is falling apart from too many changes, I have to go back over it all and fix up.

[hnla]

-07

• Removes 'li_class' var as irrelevant, as pointed out wrapper_class does the job.
• removes some over zealous attr escaping, it's not possible to escape the text var as we are receiving mixed data not just strings.

[hnla]

@r-a-y
looking at your code block suggestion above I'm not sure how that would be implemented - I question not the fact it's undeniably better and cleaner - just how quite that improves the readability / code structure of BP_Button() I/we still need that logic to build the final element to return which means those if/else blocks to construct elements, add tokens etc ?

BP_Button is starting to get hard to read.

I agree however I'm just extending what existed and to some extent although slightly more complex also slightly easier to grasp, I thought, regardless if it can be better written great but I hope we don't pass this update over simply as we haven't!

As to what I'm attempting to accommodate I'm working with:
`/bp-templates/bp-nouveau/includes/classes.php (BP_Buttons_Group)

and:
`/bp-templates/bp-nouveau/includes/activity/template-tags.php (bp_nouveau_get_activity_entry_buttons( $args ))

Manipulating the $buttons args being passed to the nouveau class which in turn calls bp_get_button()

As found in the Nouveau template pack on bp github.

[hnla]

Correct various issues with escaping data.

[r-a-y]

ray.patch implements my thoughts from comment:10.

It's backward compatible and is way cleaner than 7.patch

@hnla - Let me know what you think.

Edit - Forgot to account for data attributes from comment:10! But, you can just manually define them in the parent_attr or button_attr array.

[boonebgorges]

Ha, I was in the middle of cleaning @hnla's patch when I got an email about @r-a-y's patch.

I was going to suggest that we handle the change in syntax in a new ticket, but since the patch is written, I think it's cool to go with it. I agree that the logic and the syntax is much clearer.

@r-a-y Did you write tests to test the backward compatibility stuff? Or did you just check to see if existing buttons were rendering properly?

[r-a-y]

ray.02.patch adds support for self-closing button elements (<input>, <img>) and URL-escaping for attributes with URLs in them (src, href, formaction).

---

@r-a-y Did you write tests to test the backward compatibility stuff? Or did you just check to see if existing buttons were rendering properly?

At the moment, the latter and to test if existing unit tests broke or not.

[hnla]

We appear amongst other nomenclature changes to have changed element to button, as I tried to point out button is a bad choice, button both defines a visual style ( which we're not concerned with) and an actual real world element (equally what we're only concerned with if that choice is set as a string) and calling 'it' a button when it might be a list item and styles as a simple link - but hey ho.

I have to now go and revise the Nouveau class and functions for all the passed params names and recheck to see if all conditions actually work, have a suspicion this might throw up issues.

@r-a-y good catch with the img element that element didn't occur to me at the time.

[r-a-y]

We appear amongst other nomenclature changes to have changed element to button

I was thinking about this myself. It's a trivial thing we could easily change. Could go with 'element' and 'attr' if we wanted to.

I thought since the parent element is namespaced with parent, we might want to namespace the actual button as well. FWIW, the class is called BP_Button, the function names have the word button in them.

I was also thinking about wrapper and whether we wanted to keep that as the parent namespace, since people might be accustomed to that name.

[hnla]

FWIW, the class is called BP_Button, the function names have the word button in them.

I too thought about that, still it's a bad use of words the class should simply have been BP_Make_Stuff_Clickity

Button, and also 'tabs' cause irritation item-list-tabs really what's an item, why are they tabs don't look like tabs, list is it a list? :)

Stay with the names r-a-y it's not worth re-factoring again.

I've adjusted the Nouveau class and functions and tested a basic passing of various params to generate lists, buttons, anchors and their attrs which appears to be ok or at least haven't spotted issues. Backpat link_* atts work as I didn't update Nouveau for those. Class doesn't seem to be passing through as an array key/value pair of 'button_attr' but that might be something on my end.

We ought to get data-*="" rendering if possible.

[r-a-y]

Backpat link_* atts work as I didn't update Nouveau for those. Class doesn't seem to be passing through as an array key/value pair of 'button_attr' but that might be something on my end.

If you're using a deprecated link_* parameter, this will take precedence instead of button_attr['class']. So I'm guessing you have link_class set?

Looks like we should only use the deprecated parameter if the new parameter isn't set. Will repatch.

We ought to get data-*="" rendering if possible.

I was thinking about this some more.

Would it be clearer just to set up data attributes individually in button_attr instead?

array(
    'button_attr' => array(
        'class' => 'my-css-class',
        'data-whatever' => 'my custom data',
        'data-foobar' => 'foo'
    ),
)

[hnla]

Would it be clearer just to set up data attributes individually in button_attr instead?

Guess it's simplist the way you have it above , although I had followed Boone's lead earlier in doing key/val pairs and prefixing the data- portion, devs should know the format so lets do your version, less fiddling about to get the values out of the array.

[hnla]

If you're using a deprecated link_* parameter, this will take precedence instead of button_attrclass?. So I'm guessing you have link_class set?

Yes Nouveau is passing 'link_class' in the $buttons[] args after I set the top level function params so that's ok I can possibly concatenate those top level params with the $button ones and pass all through to the 'button_attr' I had intended regardless to update and remove those older link_* keys.

[r-a-y]

ray.03.patch fixes an issue where deprecated attribute parameters like link_class took precedence over the newer ones like button_attr['class'] as mentioned in comment:21.

Also includes unit tests. Let me know what other tests should be added.

[hnla]

Swapping over the Nouveau $button args to use 'button_attr' is ok, there was an issue but it's more that the button args need to be updated to work fully.

[hnla]

Your -03 patch checks out.

I can set both:
'button_attr' => array( 'class' => 'button-class', 'title' => 'button-title' ),
and
`'link_class => 'link-class',
'link_title' => 'link-title',`

On the same $buttons[]array()

and the new 'button_attr' key/values will have precedence over the older link_* ones, comment out the 'button_attr' and the 'link_*' ones filter through.

[r-a-y]

@hnla - Are there any other issues that you've seen during implementing in bp-nouveau?

[hnla]

@r-a-y No not really, only issues tend to be ones concerning the functions and class approaches to passing args around but that is Nouveau problem.

I think from a BP_Buttons perspective it all feels ok, likely a few more real world tests would be prudent.

[hnla]

One thing I did do today was set a button element but also passed through href link as a element attr, think - as much as it's dumb operator error - we ought to render the link href only if element is an anchor? Or do we simply ask devs to be more careful and not break the internet?!

[r-a-y]

ray.04.patch is basically the same as 03.patch, but it splits up the HTML element rendering into its own class - BP_Core_HTML_Element. This class could be used for all sorts of things outside of buttons.

[hnla]

Just a mental note while updating the Nouveau $button args template tags that perhaps we ought to use continue in the attr loop if a value is empty, save printing out empty attr & accounts for people like me wanting to visually list all the keys while setting to empty strings if no values available for transposition.

[r-a-y]

In 11112:

Core: Introduce BP_Core_HTML_Element class.

This will be used to generate HTML elements when necessary.

See #7226.

[r-a-y]

In 11113:

Core: In BP_Button class, add parameter to dynamic button filter to compare parsed button arguments.

See #7226.

[r-a-y]

In 11114:

Core: Refactor BP_Button class to use an easier syntax.

Previously, to use the BP_Button class, a dev would have to define
several parameters in order to render the HTML element for a button:

array(

'wrapper' => 'div',
'wrapper_id' => 'my-wrapper',
'wrapper_class' => 'my-wrapper-class',
'link_href' => 'hxxp://example.com',
'link_class' => 'my-link-class',
'link_id' => 'my-link-id',
'link_rel' => 'nofollow',
'link_title' => 'my-link-title'

)

This commit simplifies the syntax to:

array(

'parent_element' => 'div',
'parent_attr' => array(

'id' => 'my-wrapper',
'class' => 'my-wrapper-class'

),
'button_attr' => array(

'href' => 'hxxp://example.com',
'class' => 'my-link-class',
'id' => 'my-link-id',
'rel' => 'nofollow',
'title' => 'my-link-title'

)

)

The 'parent_attr' and 'button_attr' parameters can use any arbitrary
HTML attribute set as the array key.

This commit also means we are deprecating the older parameters listed in
the first example. However, we still support these parameters via backward
compatibility.

Fixes #7226.

[r-a-y]

perhaps we ought to use continue in the attr loop if a value is empty, save printing out empty attr & accounts for people like me wanting to visually list all the keys while setting to empty strings if no values available for transposition.

Good call. I've made this change in r11112.

In my late-night commits, I forgot to add props for this ticket. Major apologies, @hnla. Wasn't my intention to do that.

[r-a-y]

In 11115:

Tests: Switch out assertNotFalse() for assertInternalType() for new button unit tests.

assertNotFalse() does not work in PHP 5.2.

Props hnla. (Make-up props! Sorry again!)

See #7226.

[r-a-y]

In 11116:

Core: Update PHPDoc for BP_Button class.

Props hnla.

See #7226.

[r-a-y]

@hnla - I've made some attempts to correct my mistake by giving you some props attached to this ticket.

I know it's not the same, but hopefully it's better than nothing.

[r-a-y]

In 11117:

Tests: Switch out assertNotFalse() for assertInternalType() for new button unit tests, pt.2.

Props hnla.

See #7226.

[hnla]

Replying to r-a-y:

@hnla - I've made some attempts to correct my mistake by giving you some props attached to this ticket.

I know it's not the same, but hopefully it's better than nothing.

np, cheers.

[needle]

Is there a reason for wrapping buttons in <div class="generic-button"> by default? in BP 2.6.n, the "Create a -----" (Group, Site etc) buttons are simply links added to the directory nav menus. The refactoring of class BP_Button now causes these links to be wrapped and pick up the style of a button - which causes problems with the styling of the menu and its items in some themes.

[needle]

I can, of course, filter these buttons so they retain their existing markup, but it seems to me that this should be handled in core. Leaving this here for anyone who might need it:

<?php
function my_unwrap_bp_directory_button( $button_args ) {
        $button_args['parent_element'] = '';
        return $button_args;
}
add_filter( 'bp_get_group_create_button', 'my_unwrap_bp_directory_button' );
add_filter( 'bp_get_blog_create_button', 'my_unwrap_bp_directory_button' );

[needle]

Remove parent element from directory buttons

[r-a-y]

@needle - Thanks for testing!

This was an oversight on my part. Can you try needle.02.patch and see if it fixes your problem?

[hnla]

@needle good catch that passed me by head, buried in testingwhether custom args worked.

@r-a-y haven't tested patch but do we want to unset the parent_element globally?

[r-a-y]

Refreshed to fix unit test.

[r-a-y]

Replying to hnla:

@r-a-y haven't tested patch but do we want to unset the parent_element globally?

Good catch! I've made this change in needle.02.patch and to also fix an issue with unit tests.

[needle]

Replying to r-a-y:

@needle - Thanks for testing!

This was an oversight on my part. Can you try needle.02.patch and see if it fixes your problem?

@r-a-y Yup, that sorts it. Thanks!

[r-a-y]

In 11187:

Core: Fix backward compatibility for BP_Button class.

• Set the $wrapper property default back to 'div' as before (​https://buddypress.trac.wordpress.org/browser/tags/2.6.2/src/bp-core/classes/class-bp-button.php?marks=87#L82) and set the $parent_element property as an empty string as a result.
• For backward compatiblity, check if property is empty before attempting to override mapped property.

See ​https://buddypress.trac.wordpress.org/ticket/7226#comment:43

Props needle, hnla.

Fixes #7226.