Opened 9 years ago
Closed 9 years ago
#6521 closed defect (bug) (fixed)
Extended profile field name "Required" label produces odd HTML
Reported by: | dcavins | Owned by: | dcavins |
---|---|---|---|
Milestone: | 2.4 | Priority: | lowest |
Severity: | trivial | Version: | 2.3.2 |
Component: | Extended Profile | Keywords: | has-patch |
Cc: | david.cavins@… |
Description
When an extended profile field is required, we add the string "(required)" to the profile field name. The code has been formatted to make the php file easier to read, but the HTML that is output doesn't render well--leaving no space character between the field name and "(required)".
The original php:
<label for="<?php bp_the_profile_field_input_name(); ?>"> <?php bp_the_profile_field_name(); ?> <?php if ( bp_get_the_profile_field_is_required() ) : ?> <?php esc_html_e( '(required)', 'buddypress' ); ?> <?php endif; ?> </label>
produces this ugly HTML due to the extra line breaks:
<label for="field_1"> Display Name (required) </label>
This uglier php:
<label for="<?php bp_the_profile_field_input_name(); ?>"><?php bp_the_profile_field_name(); if ( bp_get_the_profile_field_is_required() ) { echo ' '; esc_html_e( '(required)', 'buddypress' ); } ?></label>
produces this nicer HTML:
<label for="field_1">Display Name (required)</label>
Can we strike a middle ground so that both the php formatting and the output HTML are acceptable?
Attachments (2)
Change History (17)
#2
@
9 years ago
- Keywords reporter-feedback added
Do the aesthetics of the HTML matter, or is this markup causing an issue involving whitespace characters? (I know there's certain edge case weirdness if you split an <a>
across multiple lines, for example.)
#3
@
9 years ago
In this case, the various returns and spaces are interpreted as no characters, so the result lacks a leading space: "Field Name(required)".
That is a good question: do the aesthetics of the php or the resultant html matter more? ;)
#4
follow-up:
↓ 5
@
9 years ago
That is a good question: do the aesthetics of the php or the resultant html matter more? ;)
This is a null question!
PHP is a service provider it processes and returns directives to the server so a document can be assembled to comply with the http request. In this sense it matters not what PHP looks like but matters very much what it returns in terms of HTML especially given factors such as white space have meaning in HTML not just within cdata blocks - natural inline spacing, describe a series of inline forground images <img></img><img></img><img></img>
you'll note an odd white spacing between elements, split those elements to new lines and this vanishes.
However human readable code always matters, especially where we mix scripting and markup.
Are we saying the white space in esc_html_e( ' (required)'
is compacted?
I'm not clear why in the original code block we have a formatting issue when rendered as markup we should be rendering simple strings from those two functions in the label element.
#5
in reply to:
↑ 4
@
9 years ago
- Keywords reporter-feedback removed
Replying to hnla:
Are we saying the white space in
esc_html_e( ' (required)'
is compacted?
No, the original php doesn't include a space in the string:
<?php esc_html_e( '(required)', 'buddypress' ); ?>
So while the two strings have a variety of other white space around them in line breaks and such, the result is:
"Field Name(required)"
So we could just add a space and stop there.
#6
@
9 years ago
- Keywords good-first-bug needs-patch added
- Milestone changed from Awaiting Review to 2.4
#7
@
9 years ago
In looking into BP, this same logic appears about a zillion times:
<?php if ( bp_get_the_profile_field_is_required() ) : ?> <?php esc_html_e( '(required)', 'buddypress' ); ?> <?php endif; ?>
It's reused so much that I think we should replace it with a template function:
<?php function bp_the_profile_field_required_label(){ echo bp_get_the_profile_field_required_label(); } function bp_get_the_profile_field_required_label(){ $retval = ''; if ( bp_get_the_profile_field_is_required() ) { $translated_string = __( ' (required)', 'buddypress' ); $retval = '<span class="bp-required-field">'; $retval .= esc_html( apply_filters( 'bp_get_the_profile_field_required_label', $translated_string, bp_get_the_profile_field_id() ) ); $retval .= '</span>'; } return $retval; } ?>
and use it like this:
<label for="<?php bp_the_profile_field_input_name(); ?>"> <?php bp_the_profile_field_name(); ?> <?php bp_the_profile_field_required_label(); ?> </label>
Let me know what you think. Also, I assumed that it makes sense to load the language file translation before applying filters. If the other direction makes more sense, please say so.
#8
@
9 years ago
Template tag makes sense to me especially if wishing to update the label in some respect and can do so from one applied filter rather than having to track down all instances just to be able to add a class or change out the span.
Where would a template tag like this be located, I'm thinking, and this may receive howls of derision :) , that it ought to be at a template level so perhaps functions like this would live logically in buddypress-functions.php
#9
@
9 years ago
Hi Hugo-
I've got a proof-of-concept patch, so I'll attach that. Since this new template tag applies to profile fields only, I had put it into bp-xprofile-template.php
. Is that a reasonable spot for it?
Thanks for your feedback!
#10
@
9 years ago
@dcavins yes I guess it is, it was just to some extent it felt like a generic function more suited to a template level placement, it was more a thought than wish. :)
#11
@
9 years ago
@dcavins
Sorry it took so long for anyone to review your patch. I think you're on the right track, but I believe there are more places in BP where a "(required)" string is used for profile fields that you'll also need to cover.
#13
@
9 years ago
I've attached a new patch. Please speak up if there's a problem with this approach. Thanks!
Rather we did: