Currently a form item (and its label) is surrounded by <DIV class="form-item"> and </DIV>.
It would be good to add a second class giving the type of form-item beside the class="form-item", for example: class="textarea" or class="radio" or class="select".
giving something like: <DIV class="form-item textarea"> wich could be referenced in a CSS file by a .textarea. When this feature is not needed nothing has to be changed in the CSS files because the .form-item definitions are applied as usual.

This will allows to easily style form-items according to their type (textarea, radio, select, ...).

(I have classified this feature under "base system" because I don't know really which component(s) has to be modified. Sorry if this is wrong).

Thanks for considering this proposition.

Files: 
CommentFileSizeAuthor
#33 form-item-wtf.patch548 bytesRobLoach
Passed: 11116 passes, 0 fails, 0 exceptions View
#19 drupal.form-element-theming-2.patch2.54 KBsime
Passed: 11116 passes, 0 fails, 0 exceptions View
#16 drupal.form-element-theming.patch2.47 KBsun
Failed: Failed to run tests. View
#13 43494_drupal.form-element-class_13.patch2.16 KBstBorchert
Failed: Failed to apply patch. View
#12 43494_drupal.form-element-class_12.patch872 bytesstBorchert
Failed: Failed to run tests. View
#11 43494_drupal.form-element-class_11.patch891 bytesstBorchert
Failed: 10931 passes, 0 fails, 4432 exceptions View
#10 43494_drupal.form-element-class_10.patch889 bytesstBorchert
Failed: 10931 passes, 0 fails, 4432 exceptions View
#6 drupal.form-element-class.patch1.41 KBsun
Failed: 7658 passes, 1 fail, 0 exceptions View
#3 drupal.form-element-class.patch1.41 KBsun
Failed: Failed to install HEAD. View

Comments

LAsan’s picture

Version: x.y.z » 7.x-dev
Component: base system » forms system
Priority: Normal » Minor
Status: Active » Fixed

Old topic with no feedback.

Moving to fixed.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

sun’s picture

Title: Allow flexible styling for form-item according to their type » FAPI: Add name/type as CSS class for form elements
Assigned: Unassigned » sun
Priority: Minor » Normal
Status: Closed (fixed) » Needs review
FileSize
1.41 KB
Failed: Failed to install HEAD. View

+1

This is what we're actually doing in a per-se default override of theme_form_element() in all of our Drupal sites. Theming forms based on these classes is *a lot* easier, because you simply can't differ between div.form-item and div.form-item in CSS.

Attached patch adds the element's #name (if set) to the .form-item container (resulting in <div class="form-item elementname">), and the element's #type to the label of radio buttons and checkboxes (resulting in <label class="radio">), which is usually needed to take the special rendering/layout of single radio buttons/checkboxes into account.

lilou’s picture

Patch still applied.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
Failed: 7658 passes, 1 fail, 0 exceptions View

Re-test.

Wim Leers’s picture

+1

This also allows for less tricky/more readable JS code.

From:
$('div.form-item[select]')
To:
$('.select')

Status: Needs review » Needs work

The last submitted patch failed testing.

SocialNicheGuru’s picture

will this also work on D5 or D6? This is a great idea.

For those of us who just want to theme a form we created in using cck like a node profile for example, I would love to easily get to the element to theme instead of having to do form manipulation heavy lifting with hook form alter.

Is there a D5 or D6 equivalent?

If something like this is already available, you can contact me and let me know. I have been looking and have not found or understood as much as I should.

Thanks,
Chris

stBorchert’s picture

Status: Needs work » Needs review
FileSize
889 bytes
Failed: 10931 passes, 0 fails, 4432 exceptions View

This patch additionally adds the classes from the element to the surrounding div. In combination with #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent. this allows you to style the complete element (including label), e.g. a checkbox with class="default".

Imo its no necessary to set the class to the label because you can style it with div.form-item.form-radio label {} for example.

stBorchert’s picture

FileSize
891 bytes
Failed: 10931 passes, 0 fails, 4432 exceptions View

Argh, its not always good to use drupals functions (e.g. form_clean_id).
Here's a retry.

stBorchert’s picture

FileSize
872 bytes
Failed: Failed to run tests. View

Hm, seems to be to early this morning. Here's the working patch.

stBorchert’s picture

FileSize
2.16 KB
Failed: Failed to apply patch. View

This gets even more complicate. Updated some style rules and js-magic to work with the new classes.

sun’s picture

Status: Needs review » Needs work

I advanced on this in one of my latest themes. Will post a new patch shortly.

sun’s picture

And, btw, the reason why we need to add those classes also to checkboxes/radios is:

<div class="form-item" id="edit-checkbox-wrapper">
  <label class="option" for="edit-checkbox"><input type="checkbox" name="checkbox" id="edit-checkbox" value="1"  checked="checked"  class="form-checkbox" /> Checkbox</label>
  <div class="description">Checkbox description.</div>
</div>

1 label here.

<div class="form-item">
  <label>Checkboxes: </label>
  <div class="form-item" id="edit-checkboxes-one-wrapper">
    <label class="option" for="edit-checkboxes-one"><input type="checkbox" name="checkboxes[one]" id="edit-checkboxes-one" value="one"   class="form-checkbox" /> One</label>
  </div>
  <div class="form-item" id="edit-checkboxes-two-wrapper">
    <label class="option" for="edit-checkboxes-two"><input type="checkbox" name="checkboxes[two]" id="edit-checkboxes-two" value="two"  checked="checked"  class="form-checkbox" /> Two</label>
  </div>
</div>

Multiple labels here.

However, my new patch probably won't need it anymore.

sun’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
Failed: Failed to run tests. View

So here is the new patch.

- Adds the element's #type as class.

- Adds the element's #name as class.

- Changes the CSS class name for fieldset descriptions to "fieldset-description".

Reasoning:

a) When implementing a custom form layout in your theme (e.g. a two-column form layout, with floating form item labels to the left), you currently have no way to target a certain kind (#type) of form item. This patch adds the class "form-item-<#type>" to all form items. That makes it easy to specifically target all elements of type "checkboxes", or all elements of type "textarea", aso. Absolutely required for advanced theming of forms.

b) In addition, the element's #name is added as "<#name>-wrapper" class to the form item. Yes, we already introduced "edit-<#name>-wrapper" CSS ids, but CSS ids must be unique and thus can only exist once on a page. In addition, those existing CSS ids are not added to the wrapping form items for checkboxes and radios.

c) When styling forms, you have all kind of ".description" classes flowing around. However, the description of fieldsets is not really comparable to regular form item descriptions: they usually contain more lengthy guidelines and explanation for all form items contained in the fieldset. Yes, it is a description, but logically and semantically, it is intended and displayed differently than those form item descriptions (usually displayed relatively small below the form element, or rendered completely differently using JS tooltips).

sun’s picture

Issue tags: +Needs themer review

.

sun’s picture

Someone up for testing?

sime’s picture

FileSize
2.54 KB
Passed: 11116 passes, 0 fails, 0 exceptions View

Updated the patch to latest HEAD, hopefully the bot doesn't get angry, out of practice.

This is a very useful patch for theming.

sime’s picture

Status: Needs review » Reviewed & tested by the community

Oops, comment cut off somehow.

I meant to add that I did the following:
-- applied to HEAD
-- visually checked form html to see the extra classes
-- ran the Form API tests
-- validated a random form (content type creation) through w3org.

akahn’s picture

Status: Reviewed & tested by the community » Needs work

This line catches my eye:

return '' . ($element['#title'] ? '' . $element['#title'] . '' : '') . (isset($element['#description']) && $element['#description'] ? '

' . $element['#description'] . '

' : '') . (!empty($element['#children']) ? $element['#children'] : '') . (isset($element['#value']) ? $element['#value'] : '') . "\n";

That's pretty gruesome. Why the need for hyper-compact, unreadable code? Since this is a theme function that designers/themers (who may not be PHP-savvy) will want to override, this code should be as friendly as possible.

What if, before the return statement, we do all the conditional stuff and store things in $legend, $description, and $children, and then concatenate all that in the return statement? That would make this a lot more readable and expressive. What do people think of this approach?

sun’s picture

Status: Needs work » Reviewed & tested by the community

@akahn: Not invented here. If we want to make those theme functions for form elements cleaner for themers, then we should tackle them all at once in a separate issue.

akahn’s picture

Point taken. Anyone think it would be worthwhile to get rid of this kind of dense, hard-to-read code from Drupal core?

sime’s picture

@akahn, my opinion is you'd prefer not to override this theme function if you could avoid it. But I take your point.

emmajane’s picture

I don't do a lot of javascript, so this patch seems overkill for my needs as a themer; however, merlinofchaos and quicksketch both suggested in IRC the idea could be useful. Hopefully they'll get a chance to look at the patch directly and add their comments. HINT, HINT, HINT. :)

quicksketch’s picture

I really like the change to include "form-item-[type]" to the wrapper, but I'm not so sure about the "form-item-[name]" change. For everything except checkboxes/radios, this is going to be nearly identical to the ID. Seems like we should either A) remove the ID entirely or B) add an ID to checkboxes/radios. New classes are great, redundant ones not so much.

To compromise, could we add a #wrapper_attributes property and then run drupal_attributes() on it and tack that onto the wrapper div? Then anyone interested could add back in the "form-item-[name]" class through a hook_elements() and an extra #process function. Of course its usefulness would be different than the original patch... maybe that change should be elsewhere, but still I'd like to see a different alternative that having an id and class attribute that are nearly identical.

sun’s picture

Given that form_clean_id() will rename any ID that appears more than once on a page, no developer should rely on those IDs, but classes instead. That issue bit me just recently when I mistakenly tried to target the username field on the user login block form. However, on the path user/login, the regular user login form is rendered already, so my login block suddenly used a different ID. So neither my JS nor my CSS did apply anymore.

quicksketch’s picture

I've had that exact same situation happen with the login form when using LoginToboggin's fancy login form. Per your description, "no developer should rely on those IDs, but classes instead," it sounds like that's the exact reason why we shouldn't have the automatic ID at all. I'm not opposed to adding more classes, I'm opposed to having an ID and Class that are identical 90% of the time. If we add this wrapper class based on the element name, we should get rid of the ID, since it serves no functional purpose that the class cannot provide.

sun’s picture

Yeah, I agree. Remaining question is: Tackle that duplicate ID/class in this issue or in a separate? I'm not entirely sure about how much CSS/JS we need to change accordingly for that, so I'd rather prefer to move that into a separate issue.

sime’s picture

I'm in support of just adding the "form-item-[type]" to make life easier 90% of the time.

As long as I can avoid overriding theme_form_element().

For example, if I could over-ride individual element types, I could then simply implement phptemplate_form_element_item() or phptemplate_form_element_radios() to just sort out the weirdness that happens from design to design. Is this out of scope?

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Does this still need more discussion?

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

The changes from removing the #edit-[name]-wrapper ID doesn't look drastic, but would clearly be dependent upon this patch. I made a separate issue in #482636: Remove "edit-[name]-wrapper" ID from form element wrappers that contains a dependent patch for removing the ID attribute entirely from all CSS and JS, though it's not removed from the markup (yet) until this patch is committed. So let's do one step at a time, get this in and finish it in the other issue.

sime, I think restructuring the form element function calls is probably not part of this issue either. It'd be a more signficant code change, but at least after this patch the need to override it should be reduced quite a bit.

RobLoach’s picture

FileSize
548 bytes
Passed: 11116 passes, 0 fails, 0 exceptions View

Although it's probably un-related I ran into this a little while ago. It's unsetting the ID of any conditional form element........

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Reviewed & tested by the community

Setting to rtbc - testbot was broken.

Dries’s picture

So #33 is the patch that should get committed?

sun’s picture

#19 it is. I've sent that patch for re-testing now.

quicksketch was so kind to already create #482636: Remove "edit-[name]-wrapper" ID from form element wrappers, so #19 is ready to go.

I'm not sure what the patch of Rob Loach in #33 is for, but it sounds like a different issue.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright. Thanks! Committed to CVS HEAD. See you in #482636: Remove "edit-[name]-wrapper" ID from form element wrappers. ;-)

Status: Fixed » Closed (fixed)
Issue tags: -Needs themer review

Automatically closed -- issue fixed for 2 weeks with no activity.