Don't show widget select box if there's only one selection
webchick - October 12, 2009 - 04:00
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | field_ui.module |
| Category: | task |
| Priority: | normal |
| Assigned: | zserno |
| Status: | needs work |
| Issue tags: | D7 UX freeze, JavaScript, Needs usability review, Novice |
Description
Every time I end up clicking this to see what other wonderful widgets might be in store, and each time I am terribly disappointed to see the answer is none.
Let's not tease people. If there aren't multiple selections, let's remove the select box.
I have a feeling this might (!) be easy to solve, so tagging as novice.
#1
Coining a tag.
#2
All that needs to be done here is something like this (pseudo-code):
if ($('option:visible', element).size() == 1) {$(element).hide()
}
The solution should probably be JavaScript-only.
#3
#4
Left a little bit of debug code in and now it works for existing fields as well
#5
The last submitted patch failed testing.
#6
IMHO, the testbot is incorrectly failing this patch.
The file is JS. It shouldn't be tested for PHP syntax.
See this issue: http://drupal.org/node/608044
Here's how I tested this patch:
- Applied patch - success
- Code review of patch - looking good. Could use a comment.
- Manual testing:
. create new content type "test"
. added boolean field (shows dropdown) - success
. added image field (doesn't show) - success
. created new content of type "test" and uploaded image - success
Looks good. Ship it!
#7
I'm not (yet) convinced that should be worked around using Javascript. It doesn't optimize the behavior of people who don't have Javascript.
#8
re Dries #7:
People without JS are out of this anyway, because they will always get a 'Select widget' dropbox filled with *all* available widgets for all field types (selecting an invalid 'field type / widget' pair then comes back with a form error). Restricting the list of widgets to the ones available for the selected field type is already done through JS (obviously).
#9
Regarding patch in #4, I don't think I see why the two
.end()calls are needed, but I might be missing it.Also, is *hiding* the select really what we want ? Simply disabling it might be less confusing.
#10
Also : It would be best to commit #578544: field creation widgets in different columns from existing fields first. This fixes a trivial WTF on colspans which should make this patch clearer (there's actually no need to mess with the 'operations' column here).
Thus, back to CNW
#11
#578544: field creation widgets in different columns from existing fields has been committed.
#12
Reworked patch from #4. Please review.
Changes:
- Got rid of code that messes with the operations column as suggested in #10.
- Instead of simply hiding select lists that hold only one option, it shows their only option as text (see attached jpg). Note: I tried to simply use the disabled attribute, but it shows no visual difference in my (mac) browsers (FF 3.5, Safari 4).
- Added some comments.
I'm a bit concerned though about the copy/paste nature of this code. Is it acceptable here? I would gladly introduce a new function to avoid copy/paste programming.
#13
"Text only" doesn't look too great, it appears misaligned, and I'm not sure we can find a set of CSS margins that can make it look aligned in all browsers.
It's a shame OSX doesn't make any visual distinction between enabled and disabled selects - win does ;-)
Dunno, maybe a disabled textfield, then ? Might require some work in field.js - so maybe this should be validated first with a hacked sreenshot by modifying the HTML in firebug...
#14
Also, any solution we come up with, we'll need to make sure the form submit callback handles the case where it receives no value for the widget.
#15
Ok, found what causes this weird behavior: if you change style of a select list then OS X browsers will use this new style even for disabled elements. So simply changing color to gray helps a lot. See:
I re-rolled the patch to use the disabled attribute on widget select boxes that hold only one option. Please try. Although I'm not sure if this problem appears only on this form thus changes made to field_ui.css might need to be moved to a higher level.
#16
Cool ! Approach looks fine, now code review:
+++ modules/field_ui/field_ui.css 24 Nov 2009 15:01:45 -0000@@ -16,3 +16,7 @@
+ color: GrayText;
Hmm, I think we use real #rgb in core.
+++ modules/field_ui/field_ui.js 24 Nov 2009 15:01:46 -0000@@ -24,7 +24,14 @@ function attachUpdateSelects(context) {
+ // Don't show widget select box if there's only one selection.
+ if (this.targetSelect.fieldPopulateOptions(options).find('option').size() > 1) {
+ this.targetSelect.removeAttr('disabled');
+ }
+ else {
+ this.targetSelect.attr('disabled', 'disabled');
+ }
2 occurrences - I think this could be moved inside fieldPopulateOptions().
+ comment is not to date with the current approach (disable, not "don't show")
+++ modules/field_ui/field_ui.js 24 Nov 2009 15:01:46 -0000@@ -48,12 +55,26 @@ function attachUpdateSelects(context) {
+ // Make sure that disabled select box values get submitted too.
+ $('#field-ui-field-overview-form', context).submit(function () {
+ $('#field-overview select[disabled]').removeAttr('disabled');
+ return true;
+ });
Sounds like a hack.
+ Won't this cause a visual blip between the moment you hit Submit and the moment the new page loads ?
I'd rather have the case where there's no 'widget type' in the incoming form values dealt with in field_ui's form handlers.
Something like :
if (!isset($form_state['values']['_add_new_field']['widget_type']) {$widget_type = the only valid widget type;
form_set_value($form['_add_new_field']['widget_type'], $widget_type);
}
in _field_ui_field_overview_form_validate_add_new() - and similar in _field_ui_field_overview_form_validate_add_existing()
I'm on crack. Are you, too?
#17
@yched: Thank you for your thorough review, I'll re-roll the patch according to your remarks.
#18
Rerolled the patch according to yched's remarks in #16.
Please review and test it.
#19
+++ modules/field_ui/field_ui.admin.inc 28 Nov 2009 10:33:02 -0000@@ -402,13 +402,20 @@ function _field_ui_field_overview_form_v
+ // If there's only one possible widget type, use that.
Please make it clear that it relates to the JS feature of disabling the select if there is only one valid widget type.
+++ modules/field_ui/field_ui.js 28 Nov 2009 10:33:02 -0000@@ -59,9 +59,18 @@ function attachUpdateSelects(context) {
+ var counter=0;
+
+ for (i in options) {
+ counter++;
+ }
Hm - can't we do better than this ? Why doesn't
options.lengthwork ?+ the code comment "Disable widget select box if selection is obvious" doesn't really apply to the (already in HEAD) case where there is 0 option to select because no field type has been selected yet.
Other than that, code looks good to me. We'd need a corresponding test in Field UI, though.
This review is powered by Dreditor.