Code

Opened 7 years ago

Closed 5 years ago

Last modified 3 years ago

#3512 closed Uncategorized (fixed)

Add "required" & "error" CSS classes to form rows in as_* methods

Reported by: Waylan Limberg <waylan@…> Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: waylan@…, real.human@…, gonz, liangent@…, andy@…, drackett@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, there is no way to identify which fields have errors or are required with CSS. The patch below adds HTML classes to the containing block element for that purpose.

Therefore, using as_tr(), a field that is both required and contains an error would begin like so:

<tr class="error required">...

I have submitted three patches for consideration as I am unsure of whether we want to to allow people creating their own as_methods (through subclassing etc) to be able to add their own classes separate from the above.

See this thread for more details: http://groups.google.com/group/django-developers/browse_thread/thread/fbd0c8cfbf1b819b/

The three patches do not yet have tests (or at least the existing tests have not been adjusted) and I haven't even looked at docs. Maybe when a decision is made, I'll tackle that. Or if anyone else wants to...

Attachments (8)

html_class1.diff (2.5 KB) - added by Waylan Limberg <waylan@…> 7 years ago.
Simple solution, easy to override, but could result in &lt;tr class=&quot;&quot;&gt;
html_class2.diff (2.7 KB) - added by Waylan Limberg <waylan@…> 7 years ago.
Simple, not over-ridable, but always clean
html_class3.diff (3.2 KB) - added by Waylan Limberg <waylan@…> 7 years ago.
Adds another argument to _as_html() but is easy to override and clean.
html_class3.2.diff (3.3 KB) - added by SmileyChris 7 years ago.
slightly improved, should use same logic for html_class2.diff if that is preferred
html_class4.diff (93.9 KB) - added by SmileyChris 7 years ago.
fixing bug in previous patch, adding tests and fixing docs
html_class4.2.diff (103.1 KB) - added by SmileyChris 7 years ago.
updated to latest svn
html_class4.3.diff (111.7 KB) - added by SmileyChris 7 years ago.
add updated tests for modeltests.model_forms too
3512.diff (6.7 KB) - added by SmileyChris 6 years ago.

Download all attachments as: .zip

Change History (46)

Changed 7 years ago by Waylan Limberg <waylan@…>

Simple solution, easy to override, but could result in &lt;tr class=&quot;&quot;&gt;

comment:1 Changed 7 years ago by Waylan Limberg <waylan@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

For html_class1.diff the example in the note should have been: <tr class="">. Sorry about that.

Changed 7 years ago by Waylan Limberg <waylan@…>

Simple, not over-ridable, but always clean

Changed 7 years ago by Waylan Limberg <waylan@…>

Adds another argument to _as_html() but is easy to override and clean.

comment:2 Changed 7 years ago by Waylan Limberg <waylan@…>

  • Needs documentation set
  • Needs tests set

My guess is the docs and tests only need to be updated to reflect the new output, but I haven't got that far yet.

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

Hey Wayne check out http://code.djangoproject.com/ticket/3515 your idea is great but I needed these options on the actual fields, and to be included regardless of how I output the form. Also added css='myfield' support to all field types.

Changed 7 years ago by SmileyChris

slightly improved, should use same logic for html_class2.diff if that is preferred

comment:4 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

Perhaps this needs some sort of merge with #3515?

comment:5 Changed 7 years ago by boxed@…

I strongly prefer this fix to the one suggested in #3515. There is a bug in the patch at line 122 though, it says:
class_list = html_class_list # Reset for each loop.
This does in fact not reset the list for each loop step, but results in "required" being appended over and over again. In my example code I tried I got class="required required required" in one case :P

If this is changed to class_list=[] everything is fine as far as I can see. I would very much like this patch to go into trunk.

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

So why did you prefer 3512 over 3515? Personally, I've found it essential to control CSS class names on a per form element basis, otherwise you can't control sizes/layout etc and also for script handlers etc. The auto generated forms aren't any use to me either except for pre-design functionality testing.

comment:7 Changed 7 years ago by boxed@…

Because with 3512 I can set styles on the label or the actual form input at will with "label .required" and "input .required" for example. With 3515 the only way to set a style on the label tag is by traversing the DOM, since only the input control got the CSS class set.

With this patch you CAN set css styles on a per form basis, just not in the model. As long as you on the form tag itself put an ID, you can individually control the styling of any item in the form with the CSS selector, for example: "#id_my_poll_form #id_title label { background: yellow; }" This will only set the color of labels inside items with the id "id_title" that are in turn inside a form "id_my_poll_form". This prevents unreasonable leakage across the MVC layers, unlike 3515 where I have to set CSS classes in the model where they really don't belong.

comment:8 Changed 7 years ago by mtredinnick

The latest patch in this sequence has a bug that would need to be fixed: using mutable objects (such as []) as default arguments to functions is wrong. They are created once at import time and exist in the module's scope (not the function body). So if you accidently change them in place, the changes just accumulate each time the function is called.

This is why the bug in comment 5 is occurring (because it is assinging, by reference, a module-level object). Pass in a default of None and check it inside the function body. If the arg is None, assign an empty list to it.

I'm still trying to make sense of all the differences between this #3515 at the moment, but I wanted to note the problem while I remember it.

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

Thanks for the tip mtredinnick. I was wondering how scoping & instancing works in Python (I'm new).

Also I agree with boxed too. I've since realised I don't like the MVC leakage my CSS patch encourages. Gets a bit long winded in the CSS but that's just unavoidable MVC overhead. One suggestion though, redo 3512 at the lower level so it works for all forms not just the as_* ones, which I rarely find myself using.

comment:10 Changed 7 years ago by Waylan Limberg <waylan@…>

IMO anyone who says having the class names directly on the form elements gives them more control needs to brush up on their css skills. Given the following html:

<li class="required"><label><input...></label></li>

I can do any of the following:

# style the wrapping block with
.required {...}

# style the label
.required label {...}

# style the form element
.required input {...}

They're called "Cascading Style Sheets" for a reason. Remember there is only one form element per wrapping block. I could see a problem if that wasn't the case, but it is.

With #3515's approach, if you want to style the wrapping block element (li, tr, p) or the label you can't. I understand some people may not be using the as_methods and thats why they like 3515, but if they're building their own html, then IMO they should be building their own html.

comment:11 Changed 7 years ago by SmileyChris

And actually, the bug is that class_list should be creating a copy of html_class_list.

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

We're getting wires crossed - I don't think anyone here lacks CSS understanding.

  1. The reason 3515's CSS control is bad is because of MVC leakage; although it's convenient, model is perhaps not the cleanest place for CSS definition
  2. #id_ CSS selectors can be used to control individual field sizes, styles etc
  3. Another alternative is to make a css class name filter for templates, I'll think some more and post thoughts about that later
  4. 3512 is the more MVC-compliant approach but needs implementing for all output methods not just the as_ functions

Nice work people now lets finish it off and hopefully get it into trunk.

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

We're getting wires crossed - I don't think anyone here lacks CSS understanding.

  1. The reason 3515's CSS control is bad is because of MVC leakage; although it's convenient, model is perhaps not the cleanest place for CSS definition
  2. #id_ CSS selectors can be used to control individual field sizes, styles etc
  3. Another alternative is to make a css class name filter for templates, I'll think some more and post thoughts about that later
  4. 3512 is the more MVC-compliant approach but needs implementing for all output methods not just the as_ functions

Nice work people now lets finish it off and hopefully get it into trunk.

Changed 7 years ago by SmileyChris

fixing bug in previous patch, adding tests and fixing docs

comment:14 Changed 7 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset

Simon, I disagree that this specific ticket needs to implement this new behaviour for all output methods. IMO, it's ready to go.

If you're after fine-grained then you can do this yourself (by testing form[fieldname].errors and form[fieldname].field.required) or with a new filter like you suggested.

comment:15 Changed 7 years ago by Waylan Limberg <waylan@…>

For those who aren't using the as_methods(), you can still check if a field is required in the template, much like you can check that a field has errors:

{% for boundfield in form %}
    {% if boundfield.errors %}
    do stuff...
    {% endif %}

    {% if boundfield.field.required %}
    do stuff...
    {% endif %}
{% endfor %}

We could always add a 'required' property to the BoundField class, but why? Here it is anyway (Allowing one to do {% if boundfield.required %} ):

def _required(self):
    "Returns True if this BoundField is required."
    return self.field.required
required = property(_required)

Whether that little patch is applied or not, those of you building your own forms can generate whatever html you want without any additional patches and that is why this patch only affects the as_methods(). Yes, if you want a class attribute on a field element itself, you have to manually build the entire field element (with boundfield.name, boundfield.data, boundfield.label etc) but isn't that the way it should be for those who want such fine grained control as to assign custom classes to individual elements in the templates?

Note: I'm posting this partially as a note to myself. I figured this all out a month ago when I wrote the patches, but promptly forgot. Following the above discussion, I dug in again only to realize I was repeating myself. D'oh.

comment:16 Changed 7 years ago by Waylan Limberg <waylan@…>

... And I see SmileyChris made the same point in less words while I was typing that. Sorry for the repitition.

Nice work SmileyChris. I'd say your latest patch is ready to go as well.


Oh and Simon,

"We're getting wires crossed - I don't think anyone here lacks CSS understanding."

Your previous comment would appear otherwise:

"Personally, I've found it essential to control CSS class names on a per form element basis, otherwise you can't control sizes/layout etc"

If I misunderstood you, sorry about that, but between that comment and some in the original thread on the mailling list, I figured I'd clear things up once and for all.

comment:17 Changed 7 years ago by mrmachine

  • Cc real.human@… added

i would like to see something similar to the old css classes that were applied to actual form elements. with oldforms we had .vPasswordField, .vEmailField, .vIntegerField, etc. that was great, as it allowed you to set smaller fields for numerical data, or larger fields for email data.

in a pinch i'd accept simplified classes based on the form field type, not the model field type. e.g. .text, .button, .checkbox, .radio, and any other types of form elements specified with the input tag. this is helpful due to some browsers lacking the more specific selectors and therefore being unable to distinguish between a checkbox or a text input.

i'd also like to see an "optional" class applied to fields that are blank=True and/or null=True. if required/error/optional must go on the containing p, tr, or td tags, i'd like to see them renamed to something a little more obscure or specific. there may be many people out there who already have generic required/error classes which are used in p and table tags but are not actually related to forms. if the classes are assigned to label or actual form elements, you know it's attached to a form. otherwise there may be some collission.

comment:18 Changed 7 years ago by mrmachine <real dot human at mrmachine dot net>

oh, and i agree that at least some kind of css class needs to be attached to every actual html form field tag - input, select, textarea - as these may be output individually without generating the entire p or li or tr around a label and an input, select, or textarea.

comment:19 Changed 7 years ago by SmileyChris

mrmachine, you're getting rather sidetracked from this ticket. If you want to suggest other changes (even if they are similar), please open a new ticket ;)

comment:20 follow-up: Changed 7 years ago by mrmachine

done. #3898. apologies ;)

comment:21 in reply to: ↑ 20 Changed 7 years ago by Gary Wilson <gary.wilson@…>

See also #3686, which aims to add the classes back to the label and input tags, which is how Django currently does things.

Changed 7 years ago by SmileyChris

updated to latest svn

Changed 7 years ago by SmileyChris

add updated tests for modeltests.model_forms too

comment:22 Changed 7 years ago by ubernostrum

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

Closing in favor of #3515.

comment:23 Changed 7 years ago by SmileyChris

  • Patch needs improvement set
  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Summary changed from [patch] Add HTML classes to fields in newforms as_methods: "required" & "error" to [patch] Add "required" & "error" CSS classes to form rows in as_* methods

After talking with ubernostrum, we came to the conclusion that this is not a direct duplicate

Because of their different tacts (independent approaches), I think it is best to leave them both open for a design decision even though they are attempting to solve the same problem.

#3515 is about attaching the class information directly to the HTML widget

#3512 is about attaching the class information to the HTML row (which you could then use to style the label or widget)

Tests need updating now that the newform tests have been split up. But I wouldn't bother doing that until I actually got a confirmation on this ticket or it'll just rot again.

comment:24 follow-up: Changed 7 years ago by Simon Litchfield

Problem with this one, is it applies only to the as_ul etc which aren't useful in real world applications.

Newforms needs built-in, object level row *and* fieldset support. Then, we can talk about CSS classing rows and fieldsets.

comment:25 in reply to: ↑ 24 Changed 7 years ago by SmileyChris

Replying to Simon Litchfield:

Problem with this one, is it applies only to the as_ul etc which aren't useful in real world applications.

It only needs to apply to the helper methods because if you're using low level methods, then you can already do this.

Just test form[fieldname].errors and form[fieldname].field.required

comment:26 follow-up: Changed 7 years ago by Simon Litchfield

It only needs to apply to the helper methods because if you're using low level methods, then you can already do this.

Yes, but what a mess of a template we end up with. The PHP and .NET guys would be laughing at us.

comment:27 in reply to: ↑ 26 Changed 7 years ago by SmileyChris

Replying to Simon Litchfield:

It only needs to apply to the helper methods because if you're using low level methods, then you can already do this.

Yes, but what a mess of a template we end up with. The PHP and .NET guys would be laughing at us.

If you're silly enough not to abstract it off to a helper method of your own, then they deserve to laugh ;) My point was that it's already possible. And aside from that, the current patch in #3515 shouldn't be attaching presentation information to the Field class - that's what the Widget class is for.

Anyway, I brought this up on django-dev again, let's discuss it there.

comment:28 Changed 7 years ago by Simon Litchfield

Think that's my point really, users shouldn't have to abstract newforms for such basic functionality. How many hoops should a user have to go through, for such simple functionality. Without row and fieldset support, newforms is half baked.

comment:29 Changed 6 years ago by telenieko

  • Summary changed from [patch] Add "required" & "error" CSS classes to form rows in as_* methods to Add "required" & "error" CSS classes to form rows in as_* methods
  • Triage Stage changed from Design decision needed to Accepted

I'm almost sure that the spirit of the as_* methods is, precisely to avoid to use a template to print all fields. Having to do such template to have the error & required attributes seems a bit... non-DRY at all.

Moving to accepted. But the patch still needs to be updated.

comment:30 Changed 6 years ago by SmileyChris

  • Needs documentation set
  • Patch needs improvement unset

Ok, here's my new take. Fully backwards compatible now.

Forms have two new attributes: error_html_class and required_html_class (both set to None by default)
When you set them, your error / required rows get given the html class. Simple. Effective. Bam.

Changed 6 years ago by SmileyChris

comment:31 Changed 6 years ago by gonz

  • Cc gonz added

comment:32 Changed 5 years ago by liangent

  • Cc liangent@… added

i need this

comment:33 Changed 5 years ago by pihentagy

Any info when it will be merged to django?

thanks

comment:34 Changed 5 years ago by guettli

  • Cc hv@… added

comment:35 Changed 5 years ago by anonymous

  • Cc andy@… added

comment:36 Changed 5 years ago by anonymous

  • Cc drackett@… added

comment:37 Changed 5 years ago by jacob

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

(In [11830]) Fixed #3512: it's now possible to add CSS hooks to required/erroneous form rows. Thanks, SmileyChris.

comment:38 Changed 3 years ago by guettli

  • Cc hv@… removed
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.