Skip to:
Content

BuddyPress.org

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's profile dcavins Owned by: dcavins's profile 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 '&ensp;';
		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)

6521-concept.01.patch (2.5 KB) - added by dcavins 9 years ago.
Proof of concept.
6525.02.patch (8.5 KB) - added by dcavins 9 years ago.
Update all '(required)' uses in profile template functions.

Download all attachments as: .zip

Change History (17)

#1 @hnla
9 years ago

Rather we did:

<span class="bp-required-field"><?php esc_html_e( ' (required)', 'buddypress' ); ?></span>

#2 @DJPaul
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 @dcavins
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: @hnla
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 @dcavins
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 @DJPaul
9 years ago

  • Keywords good-first-bug needs-patch added
  • Milestone changed from Awaiting Review to 2.4

#7 @dcavins
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 @hnla
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 @dcavins
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!

@dcavins
9 years ago

Proof of concept.

#10 @hnla
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 @DJPaul
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.

#12 @DJPaul
9 years ago

See also #6575

@dcavins
9 years ago

Update all '(required)' uses in profile template functions.

#13 @dcavins
9 years ago

I've attached a new patch. Please speak up if there's a problem with this approach. Thanks!

#14 @DJPaul
9 years ago

  • Keywords has-patch added; good-first-bug needs-patch removed

@dcavins

Some minor spacing issues around (){ but looks good other than that.

#15 @dcavins
9 years ago

  • Owner set to dcavins
  • Resolution set to fixed
  • Status changed from new to closed

In 10179:

Introduce bp_the_profile_field_required_label().

Introduce a new template function,
bp_the_profile_field_required_label(), that
allows for translation and filtration of
‘required’ label for use with extended profile
fields.

Fixes #6521 and #6575.

Note: See TracTickets for help on using tickets.