Opened 8 years ago

Closed 6 years ago

#3515 closed (duplicate)

CSS improvements for newforms

Reported by: Simon Litchfield <simon@…> Owned by: nobody
Component: Forms Version: master
Severity: Keywords:
Cc: SmileyChris, yopi, gonz, jb0t Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

Example --

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

Attachments (13)

fields.diff (905 bytes) - added by Simon Litchfield <simon@…> 8 years ago.
Diff for newforms/fields.py
fields.2.diff (1.4 KB) - added by Simon Litchfield <simon@…> 8 years ago.
forms.diff (696 bytes) - added by Simon Litchfield <simon@…> 8 years ago.
fields.3.diff (1.6 KB) - added by Simon Litchfield <simon@…> 8 years ago.
Use this one
forms.2.diff (958 bytes) - added by Simon Litchfield <simon@…> 8 years ago.
Fixed errors 'sticking'
fields.4.diff (1.5 KB) - added by Simon Litchfield <simon@…> 8 years ago.
Fixed errors 'sticking'
00-widget-outer-attrs.diff (11.1 KB) - added by Petr Marhoun <petr.marhoun@…> 7 years ago.
01-widget-outer-attrs-newforms-admin-fix.diff (602 bytes) - added by Petr Marhoun <petr.marhoun@…> 7 years ago.
00-widget-outer-attrs.2.diff (11.1 KB) - added by Petr Marhoun <petr.marhoun@…> 7 years ago.
00-widget-outer-attrs.3.diff (8.7 KB) - added by Petr Marhoun <petr.marhoun@…> 7 years ago.
01-form-row-widget-class.diff (924 bytes) - added by Petr Marhoun <petr.marhoun@…> 7 years ago.
02-widget-outer-attrs-newforms-admin-fix.diff (602 bytes) - added by Petr Marhoun <petr.marhoun@…> 7 years ago.
00-widget-outer-attrs.4.diff (8.7 KB) - added by Petr Marhoun <petr.marhoun@…> 7 years ago.
fixed a bug in Textarea.init

Download all attachments as: .zip

Change History (34)

Changed 8 years ago by Simon Litchfield <simon@…>

Diff for newforms/fields.py

comment:1 Changed 8 years ago by Simon Litchfield <simon@…>

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Summary changed from Allow CSS to be set on newforms fields to CSS improvements for newforms fields

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.

Changed 8 years ago by Simon Litchfield <simon@…>

Changed 8 years ago by Simon Litchfield <simon@…>

comment:2 Changed 8 years ago by Simon Litchfield <simon@…>

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

Changed 8 years ago by Simon Litchfield <simon@…>

Use this one

Changed 8 years ago by Simon Litchfield <simon@…>

Fixed errors 'sticking'

Changed 8 years ago by Simon Litchfield <simon@…>

Fixed errors 'sticking'

comment:3 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

Perhaps this needs some sort of merge with #3512?

comment:4 Changed 8 years ago by Simon Litchfield <simon@…>

  • Cc SmileyChris added

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.

comment:5 Changed 8 years ago by mtredinnick

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.

comment:6 Changed 8 years ago by SmileyChris

  • Resolution set to duplicate
  • Status changed from new to closed

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.

comment:7 Changed 8 years ago by SmileyChris

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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.

comment:8 Changed 8 years ago by Simon Litchfield <simon@…>

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

comment:9 Changed 7 years ago by ubernostrum

Marked #3512 as a duplicate.

comment:10 Changed 7 years ago by ubernostrum

Marked #4005 as a duplicate.

comment:11 Changed 7 years ago by ubernostrum

Marked #4932 as a duplicate.

Changed 7 years ago by Petr Marhoun <petr.marhoun@…>

Changed 7 years ago by Petr Marhoun <petr.marhoun@…>

comment:12 Changed 7 years ago by 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).

comment:13 follow-up: Changed 7 years ago by SmileyChris

  • Summary changed from CSS improvements for newforms fields to CSS improvements for newforms

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?

Changed 7 years ago by Petr Marhoun <petr.marhoun@…>

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by 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.

comment:15 in reply to: ↑ 14 Changed 7 years ago by SmileyChris

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.

Changed 7 years ago by Petr Marhoun <petr.marhoun@…>

Changed 7 years ago by Petr Marhoun <petr.marhoun@…>

Changed 7 years ago by Petr Marhoun <petr.marhoun@…>

comment:16 Changed 7 years ago by 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.

Changed 7 years ago by Petr Marhoun <petr.marhoun@…>

fixed a bug in Textarea.init

comment:17 Changed 7 years ago by yopi <yannvr@…>

  • Cc yopi added

comment:18 Changed 7 years ago by 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").

comment:19 Changed 6 years ago by gonz

  • Cc gonz added

comment:20 Changed 6 years ago by jb0t

  • Cc jb0t added

comment:21 Changed 6 years ago by SmileyChris

  • Resolution set to duplicate
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.
Back to Top