Opened 10 years ago

Closed 10 years ago

#3023 closed enhancement (fixed)

[patch] Newforms - widgets should use id and forms should use labels

Reported by: Chris Beaven Owned by: Adrian Holovaty
Component: Core (Other) Version:
Severity: normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Firstly, widgets should use id if a name is given. When I did this I cleaned up some un-DRYness in the widget code.

Secondly, forms should use labels when outputting.
The new Label widget uses the name as the for attribute.

I went through all the tests and fixed them up.

Attachments (1)

newforms.patch (38.5 KB) - added by Chris Beaven 10 years ago.

Download all attachments as: .zip

Change History (7)

Changed 10 years ago by Chris Beaven

Attachment: newforms.patch added

comment:1 Changed 10 years ago by Adrian Holovaty

A couple of comments on this patch:

  • It's sort of strange to have Label as a subclass of Widget. Although Label might share some behavior with Widget coincidentally, they're fundamentally different things.
  • I'd intentionally left id attributes off of widgets because it struck me as a bit of magic. The framework should only add things that are absolutely necessary, and auto-adding id attributes has some problems, namely that it assumes no other form on the page has that ID. Granted, Django's current form system puts in the ids automatically, but that has struck me as slightly inelegant. I see this patch only puts in the id if an ID hasn't been explicitly provided (which is nice), but it still has the problem of magically/automatically putting in the ID. I'm definitely open to hearing arguments for or against automatically putting in ids, though; I'm not super passionate about the decision.

comment:2 Changed 10 years ago by Adrian Holovaty

(In [4071]) newforms: Cleaned up some un-DRYness by adding Widget.build_attrs(). Also slightly changed flatatt to include a leading space, so the spaces don't have to be hard-coded each time you embed flatatt() results in HTML. Thanks, SmileyChris. Refs #3023

comment:3 Changed 10 years ago by Chris Beaven

Thanks for the comments.

Fair enough about Label not really being a widget, I just wanted somewhere to put it. I'd still say it could go in there without too much fuss, just broaden your view of a widget. ;)

Some id comments:

  • Ok, let's remove some magic, but we need to have a way to automatically add them in! Every elegant HTML form should use labels, and they are only supported properly across browsers by the use of for="id".
  • How about a property of the form called auto_id. If auto_id == False (which it is by default), no auto-ids. If set to a string with a %s in it, it's the default id (based on name) for any widget, any other non-false value causes the id to match the name.

While you're reading, I have some other general comments:

  • It would be much nicer if as_table and the like didn't actually include the <table> wrapper (or <ul> wrapper, etc). For most flexibility, the designer should be able to put that in so they can class it however they like.
  • ErrorDict should inherit django.utils.datastructures.SortedDict.
  • An easy way of not using errors if it's a form with no passed data would be very helpful. My suggestion is that if data is None when in the Form's initialized, then errors should be suppressed (this way, you could pass an empty dict still if you want the errors initially).

comment:4 Changed 10 years ago by Chris Beaven

For my ID suggestion, see #3025

comment:5 Changed 10 years ago by Adrian Holovaty

SmileyChris: I don't necessarily agree about using SortedDict -- what's the use-case for needing the keys of the error dictionary to be ordered? The keys simply correspond to the field names, and the order of the fields is known, so it's possible to reconstruct the error dict in order if you really want to. Besides, I can't think of any time when you'd want to.

Thanks for opening #3025. Could you open a separate ticket for the "easy way of not using errors" issue and as_table issue? Once those are opened, feel free to close this ticket.

comment:6 Changed 10 years ago by Chris Beaven

Resolution: fixed
Status: newclosed

You're right, SortedDict is not really needed.

Ticket closed since I've split out the other issues.

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