Let user pass additional param to edit_field_html() to show/hide form labels

[hnla]

This relates to: #7665

Currently we hardcode the screen reader class to the label elements in the classes.

We have no easy means of changing this from the template perspective, I would perhaps prefer to re-instate label visual display for Nouveau.

The attached patch is a proof of concept and not considered the right approach, necessarily. The current param array $raw_properties = array() ( intended for form control key/value pairs ) feels odd and suitable for refactoring, however the manner in which this is processed makes things awkward, additional args slipped in to array are fetched prior to bp_parse_args() then unset from array, feels a little dirty to mix in this way but same approach could be used for a new key pair 'show_label' => true,

This only addresses one class file for datebox at this time.

[hnla]

Initial proof of concept

[antonioeatgoat]

What about use the same concept also for the descriptions? They are surely a lower severity than labels, but it's the same concept and it could worth add both params (or array of params) at the same time.

[hnla]

@antonioeatgoat To turn on or off descriptions? I would have thought that that is something one handles from the dashboard xprofile groups simply don't state a description for the item, or have you another case that I'm missing?

[antonioeatgoat]

@hnla Yes I mean to turn on or off descriptions. Thinking as a theme developer I can think to some specific cases. For instance in a theme I used those functions to a render an advanced search form for members and I didn't want to show the descriptions of the fields. I had to add add_filter('bp_get_the_profile_field_is_required', '__return_false'); right before edit_field_html and remove_filter('bp_get_the_profile_field_is_required', '__return_false'); right after.

I don't know a specific case like this can worth the change, but is just to inform and lead to a complete discussion.

[hnla]

@antonioeatgoat Well it would be no harder to add this too, and if there's positive feedback to this ticket and others concur that the option is a valid one we can add it.

All I need is consensus on the best way of adding new $args to the edit field method as it's a bit of an odd one compared to many of our functions.

[antonioeatgoat]

What about this?

public function edit_field_html( array $raw_properties = array(), array $args = array() ) {
    ....
    ....
}

Where $args can contain both of the parameters and it remains flexible for eventual new future parameters.

[hnla]

A possible approach, personally I don't like the way we have written this from the get go but don't think it can be re-written so this example may be best.

[DJPaul]

It seems strange to me that our templates have not been using form labels correctly since day 1. I mean, this was in HTML 4.01. I think addressing that is more valuable than providing an "opt-in" parameter to control this behaviour.

This is why I built the patch on #7665 that fixes the past mistake. There are legitimate concerns about backwards compatibility, but those should be discussed in #7665.

Now we have bp-template/shared/, I am open to considering a suggestion, made previously in #5799, of moving the template-y bits of field types into partial templates. This would prevent the filter juggling that @antonioeatgoat said they've had to do. And probably keep @hnla from grumbling about this again in the future. 😊

[hnla]

keep @hnla from grumbling about this again in the future

hnla's only happy when it has something to grumble about, darned curmudgeonly beggar.

The optimal outcome here is of moving the template-y bits of field types into partial templates it's what I wanted a while back as the core implementation across ~5 class files is just frustrating and we pulled back markup from templates recently to accommodate lack of distinct field/label ID just in order to link label to input control, to my chagrin.

[hnla]

#5799 is entirely the correct sentiment @jjj is spot on here in opening remark.
I do, however, tend to feel the ticket then goes on to really over complicate the matter - backend devs :) but I may well be being unfair there, the methods/classes are a tad fiddly.

Lets rejuvenate that ticket although little scared at the work it may require but also that it's actually a pretty important task to achieve if we have the will to.

[DJPaul]

I think having a parameter to control a label is not the right fix for this problem. People may have equally good arguments for changing other parts of the markup; the right fix for that is #5799.

Semantically, the correct way to do things is have a visible label for all users. I think it's safe enough to retro-fit this into Legacy; see #7665 -- the change isn't huge, and we can target the elements with the new CSS class, to make things look like they did before.

[DJPaul]

Closing in favour of the plan proposed in #7665