Posted by Bojhan on July 18, 2009 at 4:00pm
10 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | upload.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | accessibility, Favorite-of-Dries, ui-pattern, ui-text, Usability |
Issue Summary
The text to describe the format of your maximum resolution is rather hard to read. Due to tags, this patch removes these tags and turns them into normal text.
Would be even better if we could also change the default value from 0 to 0x0.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| Before-change.png | 13.44 KB | Ignored: Check issue status. | None | None |
| After-change.png | 9.22 KB | Ignored: Check issue status. | None | None |
| width.x.height.patch | 973 bytes | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
Nitpicking really, but why not lowercasing those, while you are at it?
#2
Industry standard I think, not sure.
#3
The better solution is probably to use two textfields? Like
[ 640 ] x [ 480 ] pxor something where[ ]is a textfield?#4
Doesn't sound as a bad idea, would probably also open the doors for contrib modules to add Percentage/inches stuff.
#5
Here's a patch with two textfields.
#6
What about this?
#7
wrong patch
#8
try 3
#9
The last one don't include upload.admin.css.
#10
including the right picture now as well
#11
The last submitted patch failed testing.
#12
Fixing tests, removing .css
#13
marking,
#14
The last submitted patch failed testing.
#15
when trying to apply the last patch it said like 62 was unknown line type, so I remade the patch and looks like a tab was messing things up, change the tab into spaces and made a new patch, tried to apply it and it works
(I didn't change anything to Bojhan's patch aside from the tab)
NOTE: this solution will cause screen readers not to read anything, as the "Maximum resolution ...." label isn't set as the label for any of the 2 fields, so even though this might be a usability improvement, it certainly isn't an accessibility improvement
#16
#17
The last submitted patch failed testing.
#18
It will be impossible to label two input fields with one label, as the for attribute of a label accepts only one element ID.
Recommendations:
1. use two labels, one for each input field.
2. If necessary hide one of the labels by using style="height: 0; overflow: hidden;", so that it does not display visually, but is available to screen-reader users. This will mean that there is less of a clickable area for the second input field.
Attempting to define a system class to make content invisible in #473396: Defining System-Wide Approaches to Remove, Make Invisible & Push Content Off-screen with CSS
#19
#20
Here's another on with floated textfields and a label for each textfield.
#21
I like this solution, but I'm wondering a few things:
do we rly need to t() the 'x' and 'px' and are there any reusable classes we can utilize for the floating?
#22
Hm. Maybe someone like to translate it. Or append "pixels" instead of "px". But it wouldn't bother me if we don't wrap it in
t().I scanned current head for classes that uses
float: left;only and found.progress-disabled {float: left; /* LTR */
}
.ahah-progress {
float: left; /* LTR */
}
.password-strength-title {
float: left; /* LTR */
}
in system.css. Nothing to re-use in this issue.
#23
I like where this is going. This patch will make the selection of max width/height for images much more intuitive and user friendly.
Couple of simple code styling fixes.
+ margin-left: .4em;I think this should be 0.4em
+ if (($form_state['values']['upload_max_resolution_x'] != '0')) {and
+ if (($form_state['values']['upload_max_resolution_y'] != '0')) {These both have an extra set of parentheses that are not necessary.
Can we change the
form_set_error()messages so that they are more specific. Something like, "The maximum allowed image height should be entered as a numeric value. Set to 0 for no restriction.", and the other one to "width".+ '#field_suffix' => '<kbd>' . t('x') . '</kbd>'Should have a comma at the end, we always add a trailing comma to large arrays.
#24
Ok, updated the patch according to eojthebrave's notes and removed the useless check if the value not equals '0' around
is_numeric().#25
Lose the kdb and then I think this is RTBC
#26
Removed
<kbd>.Good enough for RTBC? :-)
#27
Yes. RTBC
#28
The div with upload-max-resolution-y-wrapper seems to be unused? No one declares the class upload-max-resolution-y-wrapper.
#29
Also, could we use #attributes to set the needed class, instead of adding yet another wrapper div with #prefix and #suffix? It's better to stay away from those as much as possible.
#30
Looks like it needs one more re-roll. :)
#31
@Dries:
upload-max-resolution-y-wrapperis generated through forms api and is used in CSS.In fact the CSS rule didn't had the effect I thought so I modified it a little.
@Damien:
'#type' => 'item'did not have a'#attributes'key. Therefore we couldn't add these classes via'#attributes' => array('class' => 'form-item-wrapper form-item-resolution'),. I tried (in fact forms api is doing its magic even if its not documented) but the classes aren't added to the element.So this has only very minor changes in upload.admin.css.
#32
@stBorchert: why not just use 'markup'? because an 'item' with a description just generates an unbound label, and markup allows the use of attributes
<?php$form['settings_general']['upload_max_resolution'] = array(
'#value' => t('The maximum allowed image size (e.g. 640x480). Set to 0x0 for no restriction. If an <a href="!image-toolkit-link">image toolkit</a> is installed, files exceeding this value will be scaled down to fit.', array('!image-toolkit-link' => url('admin/settings/image-toolkit'))),
'#attributes' => array('class' => 'form-item-wrapper form-item-resolution')
);
?>
perhaps?
#33
@seutje: 'markup' isn't wrapped into tags so we can't position it and we aren't able to place the fields beneath it.
And (additionally) we wouldn't have a title for this "field group".
Well, the label is unbound but I know no better way to build a structure like the one the last patch is providing.
#34
Can we have a screenshot?
#35
Layout didn't change since comment #20 --> screenshot
#36
Oke, to me it still sounds rather RTBC then.
#37
The last submitted patch failed testing.
#38
Ok bot, here's a new one. Especially for you.
#39
Committed to CVS HEAD. Thanks.
#40
A small follow up issue --> #556138: Error 404 for image toolkit in the File uploads setting. :-)
#41
Automatically closed -- issue fixed for 2 weeks with no activity.