Opened 9 years ago

Closed 9 years ago

#3023 closed enhancement (fixed)

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

Reported by: SmileyChris Owned by: adrian
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 SmileyChris 9 years ago.

Download all attachments as: .zip

Change History (7)

Changed 9 years ago by SmileyChris

comment:1 Changed 9 years ago by adrian

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 9 years ago by adrian

(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 9 years ago by SmileyChris

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 9 years ago by SmileyChris

For my ID suggestion, see #3025

comment:5 Changed 9 years ago by adrian

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 9 years ago by SmileyChris

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

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