Skip to:
Content

#7226 closed enhancement (fixed)

Update BP_buttons class to accept new arg param for $element_type

Reported by: hnla Owned by: r-a-y
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc:

Description

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

Attachments (13)

7226-01.patch (4.5 KB) - added by hnla 22 months ago.
First pass rough re-working to allow new args params for buttons.
7226-02.patch (5.4 KB) - added by hnla 22 months ago.
Adds name and value pairs to the atts - forms need them you know!
7226-03.patch (6.8 KB) - added by hnla 22 months ago.
Updates patch to provide ul construct logic and element rendering.
7226-04.patch (7.6 KB) - added by hnla 22 months ago.
Correct confusion as to what args 'wrapper' refered to.
7226-05.patch (7.6 KB) - added by hnla 22 months ago.
Minor adjustments, data attr esc_html.
7226-06.patch (9.6 KB) - added by hnla 22 months ago.
Updates for BP_Button class, new element type, improved data attr parsing and rendering.
7226-07.patch (8.8 KB) - added by hnla 22 months ago.
Correct various issues with escaping data.
7226.ray.patch (13.2 KB) - added by r-a-y 22 months ago.
7226.ray.02.patch (15.0 KB) - added by r-a-y 22 months ago.
7226.ray.03.patch (18.1 KB) - added by r-a-y 22 months ago.
7226.ray.04.patch (20.3 KB) - added by r-a-y 22 months ago.
7226.needle.patch (833 bytes) - added by needle 21 months ago.
Remove parent element from directory buttons
7226.needle.02.patch (753 bytes) - added by r-a-y 21 months ago.
Refreshed to fix unit test.

Download all attachments as: .zip

Change History (63)

#1 @boonebgorges
22 months ago

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.

#2 @hnla
22 months ago

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
22 months ago

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

#3 @hnla
22 months ago

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
22 months ago

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

#4 @hnla
22 months ago

  • Keywords has-patch added; needs-patch removed

-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
22 months ago

Updates patch to provide ul construct logic and element rendering.

#5 @hnla
22 months ago

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.

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

@hnla
22 months ago

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

#6 @hnla
22 months ago

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
22 months ago

Minor adjustments, data attr esc_html.

#7 @boonebgorges
22 months ago

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.

#8 @hnla
22 months ago

@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
22 months ago

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

#9 @hnla
22 months ago

-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.

#10 @r-a-y
22 months ago

@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.

#11 @hnla
22 months ago

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.

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

#12 @hnla
22 months ago

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.

#13 @hnla
22 months ago

-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.

#14 @hnla
22 months ago

@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.

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

@hnla
22 months ago

Correct various issues with escaping data.

#15 @r-a-y
22 months ago

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.

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

@r-a-y
22 months ago

#16 @boonebgorges
22 months ago

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?

#17 @r-a-y
22 months ago

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.

@r-a-y
22 months ago

#18 @hnla
22 months ago

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.

#19 @r-a-y
22 months ago

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.

#20 @hnla
22 months ago

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.

#21 @r-a-y
22 months ago

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'
    ),
)
Last edited 22 months ago by r-a-y (previous) (diff)

#22 @hnla
22 months ago

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.

#23 @hnla
22 months ago

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.

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

#24 @r-a-y
22 months ago

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.

@r-a-y
22 months ago

#25 @hnla
22 months ago

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.

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

#26 @hnla
22 months ago

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.

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

#27 @r-a-y
22 months ago

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

#28 @hnla
22 months ago

@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.

#29 @hnla
22 months ago

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?!

#30 @r-a-y
22 months ago

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.

@r-a-y
22 months ago

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


22 months ago

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


21 months ago

#33 @hnla
21 months ago

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.

#34 @r-a-y
21 months ago

In 11112:

Core: Introduce BP_Core_HTML_Element class.

This will be used to generate HTML elements when necessary.

See #7226.

#35 @r-a-y
21 months ago

In 11113:

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

See #7226.

#36 @r-a-y
21 months ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

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.

#37 @r-a-y
21 months ago

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.

#38 @r-a-y
21 months ago

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.

#39 @r-a-y
21 months ago

In 11116:

Core: Update PHPDoc for BP_Button class.

Props hnla.

See #7226.

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

@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.

#41 @r-a-y
21 months ago

In 11117:

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

Props hnla.

See #7226.

#42 in reply to: ↑ 40 @hnla
21 months ago

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.

#43 @needle
21 months ago

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.

#44 @needle
21 months ago

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' );

#45 @DJPaul
21 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@needle
21 months ago

Remove parent element from directory buttons

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

@needle - Thanks for testing!

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

#47 follow-up: @hnla
21 months ago

@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
21 months ago

Refreshed to fix unit test.

#48 in reply to: ↑ 47 @r-a-y
21 months ago

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.

#49 in reply to: ↑ 46 @needle
21 months ago

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!

#50 @r-a-y
21 months ago

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

In 11187:

Core: Fix backward compatibility for BP_Button class.

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

Props needle, hnla.

Fixes #7226.

Note: See TracTickets for help on using tickets.