Skip to:
Content

BuddyPress.org

Opened 11 years ago

Closed 6 years ago

#5184 closed enhancement (maybelater)

creating a placeholder on drop down lists on profiles

Reported by: haykayltduk's profile haykayltduk Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords: needs-patch, trac-tidy-2018
Cc:

Description

Hi,

If you add a field to the registration profile which is a drop-down list and you would like the first item to be 'Please select an option' *and* you mark the field as required, users do not have to do anything.

They are not forced to 'select an option'.

This is because when generating the HTML <option> code, a value is placed there. If, instead, the value was set to "", then this feature code work.

Look through the code, there is already the notion of 'is default'. I have a patch which adds the notion of 'is placeholder'.

Is this wanted in the core?

Thanks,
Anand

Attachments (6)

full-patch (4.7 KB) - added by haykayltduk 10 years ago.
the full patch
0001-Add-a-function-to-return-the-placeholder-text.patch (1.5 KB) - added by haykayltduk 10 years ago.
factor the string into a function
0002-Escape-the-returned-placeholder-value.patch (1.0 KB) - added by haykayltduk 10 years ago.
escape the returned value
0003-Add-the-ability-to-specify-the-placeholder-text-for-.patch (4.0 KB) - added by haykayltduk 10 years ago.
display, save and set the 'placeholder' meta value
0004-Return-the-placeholder-text-for-a-field-if-it-exists.patch (956 bytes) - added by haykayltduk 10 years ago.
if the field has placeholder, return that; or default to the existing text
5184.patch (5.0 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (23)

#1 @boonebgorges
11 years ago

Sure, I think we'd be happy to see a patch. How do you plan to implement this in the UI?

Maybe instead we should separate the text of the dropdown options from the values? Something like:

Option 1: [ Foo ] (advanced)

The advanced link would unhide a Value field for that option. By default, we'd have some JS that would copy the option title (the current "value") over to the value field, but it'd then be possible for users to change the values to something else. This would include empty strings.

But anyway, that's just an idea. Would love to see your patch, haykayltduk.

#2 @DJPaul
11 years ago

  • Component changed from Core to XProfile
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#3 @haykayltduk
10 years ago

In discussion with @DJPaul we identified that this was actually two, separate issues:

Issue 1:

For a select field, marked as optional, it should be possible for the value to be stored as null (this already works)

For a select field, marked as required, it should *NOT* be possible to proceed until a selection has been made.

What was previously happening was that a select field marked as required *would* and store the first option in the list.

Issue 2:

Rather than having the placeholder text as '----', there should be a way to customise what is displayed at a field level.

#4 @DJPaul
10 years ago

  • Milestone changed from Future Release to 1.9

#5 @DJPaul
10 years ago

See r7599 for a commit to fix Issue 1 -- not sure why it's not showing up here yet.

@haykayltduk
10 years ago

the full patch

@haykayltduk
10 years ago

factor the string into a function

@haykayltduk
10 years ago

escape the returned value

@haykayltduk
10 years ago

display, save and set the 'placeholder' meta value

@haykayltduk
10 years ago

if the field has placeholder, return that; or default to the existing text

#6 @haykayltduk
10 years ago

  • Keywords has-patch added; needs-patch removed

I've added the patch (in full patch) that should fix issue 2.

I've also attached the individual git commit as well; I think 1, 2, and 4 are simple and uncontroversial. Any feedback on patch 3 would be really be valuable.

Thanks,
Anand

#7 @DJPaul
10 years ago

  • Keywords needs-patch added; has-patch removed

Hi Anand,

Great patch. Thanks for your contributions at the WordCamp London contributor day. A few tweaks are needed, and I've spoken to you about these in person, but for the record here:

  • Set up a "proper" dev environment so it's easier to test changes out :)
  • wp_filter_kses( $_POST["placeholder_{$field->type}"] ) should use sanitize_text_field() and maybe stripslashes().
  • Some minor code style improvements -- spacing, tabs, etc, but we can fix when we commit.
  • PHPDoc for new function- -- again, we can add when we commit if needed.

Others: do we think we can get this into 1.9, or wait for 2.0?

#8 @johnjamesjacoby
10 years ago

Interesting idea. Can we get a unified patch to test and review?

Version 0, edited 10 years ago by johnjamesjacoby (next)

#9 @r-a-y
10 years ago

Thanks for the patches, haykayltduk.

I haven't tested the patches yet, but some quick thoughts:

  • The new function should be in bp-core-functions.php because we could potentially use the placeholder in other form contexts (like the settings component, 3rd-party plugins, etc.).
  • For patch 0002, any reason why we can't apply esc_html() directly in the new function?
  • For patch 0003, why isn't the new function used in line 763 of bp-xprofile-classes.php?

#10 @DJPaul
10 years ago

Look at full-patch​.php which should be a merge of the other, smaller files.

@r-a-y: it's generally better to escape near output. e.g. We might want to use this get function's return value for an attribute in one place, or in a <p> tag, or inside a textarea, for example, and these require different output escaping functions.

Although if this merits becoming a directly-usable-inside-templates function for a specific use case (instead of the more general use you seem to be hinting at), then it should be escaped in the function (likethe_title(), the_content(), etc.

Last edited 10 years ago by DJPaul (previous) (diff)

#11 @boonebgorges
10 years ago

  • Milestone changed from 1.9 to 2.0

5184.patch updates full-patch.php to do the following:

  • actually display the saved placeholder text when editing the field (this hadn't been included in the original patch)
  • format as an svn patch so we can see highlighting in trac

I think the basic functionality is good here, but there are a couple areas of concern for me.

  1. IMO, the UI needs some rethinking. Maybe this is my technical knowledge peeking through, but the word "placeholder" to me denotes the greyed-out text that appears in a text field before you start typing, like the HTML5 placeholder element. What you're proposing here is different from that (though it could overlap by expanding the patch a bit). It feels to me like the "placeholder" option should appear alongside the other options in the sortable list. For one thing, this'd make it possible to put the null-option somewhere other than the top of the list. For another, it'd highlight the fact that the null-option is *just another option*, but it so happens that it has a null value. I already suggested having a more robust [Advanced] tab in my comment above, but if that's too heavy a lift, then maybe another set of radio buttons for "empty value", like we have for "default value".
  1. I don't think the null option makes sense for checkbox fields.
  1. Could/should this be expanded to support the 'placeholder' attribute for text fields? Maybe it's a subject for a different ticket.

Let's try to get this together for 2.0.

@boonebgorges
10 years ago

#12 @boonebgorges
10 years ago

  • Milestone changed from 2.0 to 2.1

On reflection, I think this is blocked by #5220. Let's do this first thing for 2.1.

#13 @DJPaul
10 years ago

  • Milestone changed from 2.1 to 2.2

Moving to 2.2 due to lack of time. The patch is still pretty solid, and just needs some refinement and attention to get in.

#14 @DJPaul
9 years ago

  • Milestone changed from 2.2 to 2.3

#15 @DJPaul
9 years ago

  • Milestone changed from 2.3 to Future Release

#16 @DJPaul
6 years ago

  • Keywords trac-tidy-2018 added

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: https://make.wordpress.org/chat/

#17 @DJPaul
6 years ago

  • Milestone Awaiting Contributions deleted
  • Resolution set to maybelater
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.