Opened 11 years ago
Closed 7 years ago
#5184 closed enhancement (maybelater)
creating a placeholder on drop down lists on profiles
Reported by: | 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)
Change History (23)
#2
@
11 years ago
- Component changed from Core to XProfile
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#3
@
11 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.
#5
@
11 years ago
See r7599 for a commit to fix Issue 1 -- not sure why it's not showing up here yet.
#6
@
11 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
@
11 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 usesanitize_text_field()
and maybestripslashes()
.- 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?
#9
@
11 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
@
11 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.
#11
@
11 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.
- 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".
- I don't think the null option makes sense for checkbox fields.
- 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.
#12
@
11 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
@
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.
#16
@
7 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/
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.