Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow up to #1174694: Allow FAPI usage of the placeholder attribute
Add widget settings for the new #placeholder FAPI property, in the widgets for which it is relevant (textfield and textarea widgets for text, number and term fields).
The shape of options and corresponding UI will probably need some thinking. Some use cases :
- no placeholder
- instance label as #placeholder rather than regular #title
- custom text as #placeholder rather than or in addition to instance label as regular #title
Comment | File | Size | Author |
---|---|---|---|
#48 | 1241938-48.patch | 19.27 KB | swentel |
#48 | interdiff.txt | 7.47 KB | swentel |
#45 | 1241938-45.patch | 19.47 KB | swentel |
#45 | interdiff.txt | 8.25 KB | swentel |
#44 | 1241938-43.patch | 13.93 KB | swentel |
Comments
Comment #1
sunComment #2
JacineAdded to http://drupal.org/node/1183606.
Comment #3
ericduran CreditAttribution: ericduran commentedsub.
Comment #4
idflood CreditAttribution: idflood commentedsub
Comment #5
Bojhan CreditAttribution: Bojhan commentedComment #6
ericduran CreditAttribution: ericduran commentedComment #7
litwol CreditAttribution: litwol commentedAlthough i imagine #placeholder support would come equipped with backwards compatibility JS support.
Comment #8
RobLoachShould have it in Form API before we stick the support for it into Field API: #1174694: Allow FAPI usage of the placeholder attribute.
Comment #9
RobLoachOh, it was committed? Well then.
Comment #10
ericduran CreditAttribution: ericduran commentedPlaceholder support is already in form api. It was probably the 1st html5 patch to get in, a while back.
This issue is about adding placeholder support in the field api aka when someone creates a new field, and you get the option to add labels, default text, and now also placeholder :)
Comment #11
ericduran CreditAttribution: ericduran commentedOh and no, backwards compatibility is not included. Even thought is probably the simplest fallback ever.
But so far none of the html5 feature have had backwards compatibility added to them.Anyways this should be discuss in a separate issue.
Comment #12
Everett Zufelt CreditAttribution: Everett Zufelt commentedI think that this can be simplified to:
- No placeholder (default)
- Instance label (Use label as placeholder)
- Custom text
How do we communicate, through the UI, what a 'Placeholder' is, and what it does?
Comment #13
ericduran CreditAttribution: ericduran commentedIt should be clear to everyone that placeholder is not in anyway a replacement for title/label. Placeholder is a completely different attribute and should in no way have anything to do with the title/label.
So what should be expected in this patch is another text field, with the Title of "Placeholder". Now the description can be for now be what's explain in the actual spec which does a good job -- "The placeholder attribute represents a short hint (a word or short phrase) intended to aid the user with data entry. A hint could be a sample value or a brief description of the expected format. ".
This way the user chooses what to put in the placeholder we don't need to have any default or anything like that. The default is nothing if nothing is entered :)
Comment #14
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedHere is a first try to implement it for the text field type. Would it be done like that for the other field types, as well?
Comment #15
yched CreditAttribution: yched commentedThanks for attacking this :-)
As a 1st remark, though, this would rather be a widget setting (for the 'text_textfield' widget) than an instance setting. Supporting placeholders is a property of the widget, not of the field type. For instance, a slider widget on a number field wouldn't support placeholders, while a basic textfield widget would.
Comment #17
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for looking at it. Yeah ... I suspected something like that, so I only did it for text, first. Rather like this?
Comment #18
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedComment #19
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSo ... here's the same thing for all the relevant widgets.
Comment #20
yched CreditAttribution: yched commentedLooks good for a start :-), but, as I wrote in the OP, I think we need a couple more options.
@Everett Zufelt's proposals in #12 sound good.
This could be a set of 3 radio buttons :
Placeholder:
o No placeholder (default)
o Instance label (Use label as placeholder) [note: that probably means we also hide the regular #title]
o Custom text (selecting this reveals a textfield input for the custom text, through the #states API
Internally, this means two settings:
'placeholder' : 'none', 'label', 'custom', defaults to 'none'
'placeholder_custom': (only used if the above is 'custom'), defaults to ''
Comment #21
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@yched: I think #13 makes a legitimate point about that, though.
Comment #22
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedReroll with the new default settings.
Comment #23
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedReroll to make this apply after #1540174: Some attributes not allowed for the new HTML5 input elements.
Comment #24
ericduran CreditAttribution: ericduran commentedBumping and tagging for myself :)
Comment #25
ericduran CreditAttribution: ericduran commented#23: 1241938-field-placeholder-23.patch queued for re-testing.
Comment #28
swentel CreditAttribution: swentel commentedThis is so cool, let's get this in asap! Patch (+tests) takes the approach so widget plugins only need to add the 'placeholder' setting to their settings and it will be available automatically on the field instance settings screens and on the entity forms. I think this is a nice DX and also makes sure we don't have to copy the same $placeholder element form to every widget form of every field type. It *could* be a helper function of course, but I really like this approach.
I also agree with #13 as well. Let's make this initially easy and not confusing for users.
And some screenshots.
@ericduran Hope I didn't make you waste valuable time, I initially read the date wrong - I though you assigned it 4 months ago :)
Comment #29
yched CreditAttribution: yched commentedThanks for reviving this, swentel !
I'd rather not hardcode behavior on the presence of a specific "setting", though. What is in 'settings' is the playground of each widget type, what is in there and the meaning it has is entirely up to them, and the system applies no hardcoded semantics on the presence or absence of a setting with a given name - that's why those properties were separated in a 'settings' entry in the first place, keeps "who owns what" cleanly separated, I'd rather keep it that way.
Typically in our current way of working it would be a property in the widget_info (annotations), 'supports_placeholders' = TRUE or something.
However, we're trying to *remove* stuff from there rather than add more. So I'd suggest controlling it by a boolean $supportsPlaceholders property in WidgetInterface.
Comment #30
yched CreditAttribution: yched commentedOr it stays a 'setting', but the supporting code is taken care of by each widget separately :-)
Comment #31
swentel CreditAttribution: swentel commentedHmm I agree that it might be annoying and even bad DX that a potential key in your settings is reserved. But I don't like the code duplication either. Hmm, hmmm, let me think about this. Food first, I need to eat something today as well :)
Comment #32
swentel CreditAttribution: swentel commented(while eating) Not immediately a big fan of that. Some stuff then lives in annotations, other as properties. It's better DX in a way than to know all allowed properties in annotations, but it feels so schizofrenic to me. If we introduce properties, it then feels like we should remove 'default_value' as well, unless that's the plan :) (eating further)
Comment #33
yched CreditAttribution: yched commentedYes, that's totally the plan :-) - that and 'multiple_values' (for which I opened #1846162: Cleanup Widgets API : add a separate base class for 'multiple' widgets already)
I'd personally be fine with just putting 'support_placeholders' in the annotations for now, and cleanup later along with the other two, but I fear that's going to make it more difficult to get the patch in :-(.
Comment #34
swentel CreditAttribution: swentel commented(almost done eating). I'm almost thinking to let this live in settings but let every widget handle itself. This would drop (kind of ugly) following code:
This way, widgets are fully responsible like you also suggested. Also, we're in 'trouble' in case of the 'Link' field that prints 2 text fields in its widget.
This patch then isn't an (small) API change anymore and probably much easier to get in and no hassle afterwards to refactor or whatever :)
Comment #35
yched CreditAttribution: yched commentedI think I agree. Except for basic fields like text and number, other field types that want to have placeholders will most likely be more complex.
Comment #36
swentel CreditAttribution: swentel commentedAnd there we go :)
Comment #38
swentel CreditAttribution: swentel commentedFixing tests.
Comment #39
sunThis looks great. I agree it makes more sense to rather get this functionality in now. We can think about potential ways to automate/harmonize it in a separate follow-up issue.
I'd RTBC this patch, but it looks like Link field is missing?
Comment #40
swentel CreditAttribution: swentel commented@sun two placeholder fields then for the title and the link ?
Comment #41
swentel CreditAttribution: swentel commentedAlso on link, screenshot attached as well :)
Comment #42
sunThanks!
And I guess that Link field widget pretty much clarifies why an automated/harmonized solution for all widgets will be hard to achieve ;)
Awesome, let's get this in! :)
Comment #43
yched CreditAttribution: yched commentedHm, sorry, something doesn't look right: for text, number, email ... the placeholder feature is implemented as widget settings (as it should), but for link it's implemented as (instance) settings of the field type ?
Also, field_ui's ManageFieldsTest doesn't look like the right place to test this, since it's a feature of individual widgets.
This being said, I don't see us adding separate tests for each and every widget... :-(
Comment #44
swentel CreditAttribution: swentel commentedCrap, you're right, I keep confusing those two so heavily. Attached. Gimme another hour with tests for every widget!
Comment #45
swentel CreditAttribution: swentel commentedThat even didn't take an hour .. :)
Comment #46
sunLooks even better! :) Thanks!
Comment #47
yched CreditAttribution: yched commentedOk, last nitpicks :-)
link_field_widget_info() should provide default values for the settings :
and then the isset checks when using the settings are not needed :
I don't think we need to mention the notion of 'attribute' in the UI. Proposal :"The placeholder is a short hint..." ?
Comment #48
swentel CreditAttribution: swentel commentedOkidoki :)
Comment #49
yched CreditAttribution: yched commentedThanks :-)
RTBC when it comes back green !
Comment #50
andypost+1 to RTBC
PS:
Suppose this setting should be a part of each text input like format for textarea. Let's discus this in follow-ups, probably we need a change on element level
Comment #53
webchickWow, what a lovely little patch! :D
Committed and pushed to 8.x. Thanks!
Comment #54
ericduran CreditAttribution: ericduran commentedWow, that was crazy fast. I blink and it was done! :)