CSS improvements for newforms

[Simon Litchfield <simon@…>]

Example --

myfield = forms.DateField(required=True, css='vDateField')

[Simon Litchfield <simon@…>]

Diff for newforms/fields.py

[Simon Litchfield <simon@…>]

After reading the following thread, I decided to incorporate Waylan Limberg's idea of including 'error' and 'required' in the css where the field fails validation and/or if the field is required.
​http://groups.google.com/group/django-developers/browse_frm/thread/fbd0c8cfbf1b819b

Instead of including in the tr, p, etc of the as_table, as_p etc functions, I've included at the field/widget level, so they work regardless of how you output the form. Note this means I've also put the css class directly on the 'actual field tag'.

Looking forward to some feedback. I'm brand new to Django (and Python!), this is my first contribution.

[Simon Litchfield <simon@…>]

Hmm I can't change the original description.... do I need to post a new ticket?

[Simon Litchfield <simon@…>]

Use this one

[Simon Litchfield <simon@…>]

Fixed errors 'sticking'

[Simon Litchfield <simon@…>]

Fixed errors 'sticking'

[Chris Beaven]

Perhaps this needs some sort of merge with #3512?

[Simon Litchfield <simon@…>]

For sure SmileyChris :-) Already done tho mate. I've incorporated Waylan's ideas (class="error...", class="required...") and mine (class="whatever" using css="whatever" parameter to Field init). At the lower level too, so they are outputted regardless of how you output the form (manually aswell as using the as_html etc), reason for this, I'm mostly forced to do manual forms rather than automatic ones.

Design wise, I think css control (css="bla") for forms is imperative, and the approach I've taken here seems the 'right' way to go about it.

The error (and required) are very useful alternatives to the default outputting of errors too. In fact, I've had to use them already in a site I'm doing which has a tight design; the error css class works much nicer in this case than spitting text errors all through the form. Only possible enhancement here might be that we could include control over "error" and "required", perhaps in form meta.

[Malcolm Tredinnick]

There seem to be a lot of patches here and not all of them are touching the same files. Which one(s) are the current ones? Please upload a single patch that affects all the necessary files to make it easy to review the proposed change.

[Chris Beaven]

Simon has stated over in #3512 that he see flaws in the logic of this patch. I can't see any advantage of this over that patch, so I'm closing this one in favour of that one.

[Chris Beaven]

In duplicate ticket #3686 (has a patch similar to the ones here), Ubernostrum is pro this method as opposed to #3512 so I'll reopen.

[Simon Litchfield <simon@…>]

Thx smileychris, I'll have another think on it - there's more than one way to skin this cat.

[James Bennett]

Marked #3512 as a duplicate.

[James Bennett]

Marked #4005 as a duplicate.

[James Bennett]

Marked #4932 as a duplicate.

[Petr Marhoun <petr.marhoun@…>]

Why should fields care about html? Their job is validation - widgets are for html.

I don't feel that patches from #3512 and #3515 are strong enough. Can I do this with them?

• All rows with a date field have yellow background.
• All rows with errors have red background.
• One special row has green background.
• All required labels are bold.
• All labels for a password field have blue background.
• But one of them has black background.
• All password fields are 10em wide and all text fields are 20em wide.

I would like to propose new attribute for Widget.init - outer_attrs. Widgets wouldn't use it directly - it would be for "outer renderer" (BaseForm._html_output or Fieldline and AdminField in newforms-admin or whatever.)

I think that in 99 % situations only three pieces information are necessary - if row's field is required, if it has error and which kind of widget is used. So I also propose new class attribute for Widget - default_outer_class. With cascade I could use them for row, labels and fields.

With my patch there would be quite good default values for html classes and it would be very flexible. But it is new parameter and it is backward-incompatible.

It is only idea - so there are no tests (and no documentation).

[Chris Beaven]

I'm kinda liking this idea, and you are right - the field shouldn't be handling this. PS: your patch looks like it breaks as_ul.

I suggest row_attrs as a slightly more descriptive alternative (just throwing it out there).

I'm not that keen on the default_outer_class idea, and I remember negative feedback from some committers too on this behavior.

What are the backwards incompatibilities?

[Petr Marhoun <petr.marhoun@…>]

Replying to SmileyChris:

PS: your patch looks like it breaks as_ul.

Fixed.

I suggest row_attrs as a slightly more descriptive alternative (just throwing it out there).

It was my first idea too. But for example in newforms-admin a form has some fieldsets, a fieldset has some fieldlines and a fieldline has some "admin fields" (label + field). These outer attributes could be used for these admin fields (or for fieldlines AND for admin fields - at least html class). "Outer" is more general name "rows". But it is the least important thing in the patch.

I'm not that keen on the default_outer_class idea, and I remember negative feedback from some committers too on this behavior.

Use cases from my previous comment was quite crazy. But I try something real:

• All text, password and select field should be 20 em wide (and it should work also in MSIE, css selectors can't be used).
• But date field should be only 10 em wide and it should be possible to find them by javascript for adding of a calendar.
• Submit field should be 15 em wide.
• Checkbox fields and radio fields should not have any special style.

I think it is difference if there is default value for attrs and for outer_attrs. Outer_attrs are not necessary, it is quite easy to write "outer renderer" which ignore default_outer_class. But in many situations it is very useful to know something about widget in row.

What are the backwards incompatibilities?

Parameter outer_attrs can be the second or the last parameter. If it is the second parameter (it is more consistent), all calls for Widget.init with more positional arguments are broken.

[Chris Beaven]

Replying to Petr Marhoun <petr.marhoun@gmail.com>:

"Outer" is more general name "rows". But it is the least important thing in the patch.

Agreed. I'm not that worried about it :)

I'm not that keen on the default_outer_class idea, and I remember negative feedback from some committers too on this behavior.

Use cases from my previous comment was quite crazy. But I try something real:

I know there are use cases for it, but I think you are hurting the proposal by including it from the first step. How about you leave it out, and we try to get the base functionality in there first. Otherwise it's another decision that has to be agreed on before we get anywhere!
You could just as easily write your own outer renderer which built the default outer class based on the class name ;)

• Checkbox fields and radio fields should not have any special style.

Side note: I think this should really be fixed in the checkbox renderer code, so that the <ul>s have a class name. There are other tickets around for improving on that renderer.

What are the backwards incompatibilities?

Parameter outer_attrs can be the second or the last parameter. If it is the second parameter (it is more consistent), all calls for Widget.init with more positional arguments are broken.

Cool, that's what I thought. So the decision here is whether it is worth breaking the positional arguments vs putting it as the last parameter. Personally, I'd vote for breaking positional arguments too.

Could bring this topic up on django-dev please? These tickets have been sitting around for 9 months not going very far, perhaps this can help add some momentum.

[Petr Marhoun <petr.marhoun@…>]

• 00-widget-outer-attrs.3.diff: Real patch.
• 01-form-row-widget-class.diff: It is not subject of this ticket, based on idea/joke from SmileyChris - row's html class is name of widget's python class.
• 02-widget-outer-attrs-newforms-admin-fix.diff: The proposed change is not fully backward-compatible and breaks newforms-admin in one place. This patch fixes it.

[Petr Marhoun <petr.marhoun@…>]

fixed a bug in Textarea.init

[Petr Marhoun <petr.marhoun@…>]

I tried to add support for fieldsets and formsets to the trunk and it forced me to think about generation of html code in newforms from the bigger perspective. So there are new tickets for these problems - #6630, #6631 and #6632. All use inner class Meta (as in django.db.models.Model) so there are quite different. But they would fix this ticket (and also #3111 and #3512).

Customization of generated html code

From my last comment to the first ticket (#6630):

Forms with fieldsets could be very complex. And I think that they have to be customizable to be really useful. Now I can have everything or nothing - I can use prepared methods (as_table, as_ul and as_p) or do everything myself (through new methods or in templates). I think patch for adding of fieldsets has to answer three questions - what shall I do when I need to change html code for only one widget, for only one row or for only one fieldset? And there should be two kinds of answers - simple (usual situation - how to mark them for css or javascript) and complex (complete customatization).

First kind of answers: Use argument "attrs" of Widget.init for customization of one widget; Use argument "row_attrs" of Widget.init for customization of one row; Use option "attrs" of fieldset (an item in fieldset dictionary) for customization of one fieldsets.

Second kind of answers: Rewrite method Widget.render for customization of one widget; rewrite method Form._row_html_output for customization of one row (test row through bf.name or something similar); rewrite method Form._fieldset_html_output for customization of one fieldset (test fieldset dictionary through an item).

Changes in default html code

#6631 is a metaticket which proposes some possibly useful options. One is required_row_class (default value "required"), one is error_row_class (default value "error").

[Chris Beaven]

This can be marked as a duplicate of #3512, considering what that has mutated into.