#15244: Documentation needed for RadioFieldRenderer
-----------------------------------------+----------------------------------
Reporter: SmileyChris | Owner: lkeijser
Status: assigned | Milestone:
Component: Documentation | Version: SVN
Resolution: | Keywords: renderer widget
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 |
-----------------------------------------+----------------------------------
Changes (by gabrielhurley):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
First off, thank you for the patch.
Second, in the future please don't mark your own patch as "Ready For
Checkin"--when you upload a patch you should mark the "has patch"
checkbox, and then once it has been peer-reviewed the reviewer will either
provide feedback on the ticket or decide it looks good and mark it as RFC.
Before I get to the patch itself, I'd like to point out that the existence
of `RadioFieldRenderer` is inconsistent in and of itself. All the other
widgets contain the rendering logic inside their own `render` methods, and
you would customize them at that point. Even the other "special case"
input type--`select`--doesn't get a special "renderer" class. I see why
the renderer class is useful, it's just inconsistent.
As for the patch itself:
1. I would rather see this information added to the
[http://docs.djangoproject.com/en/dev/ref/forms/widgets/ widgets reference
documentation] instead of creating a new how to page. I've always been
surprised that there wasn't a topic guide on widgets, but that's the
subject of a different ticket. Right now, when people want to know about
widgets, they go to `ref/forms/widgets` and there's already information
about "customizing widgets" there... so let's keep the information
together.
2. Because `RadioFieldRenderer` is a completely one-off solution, more
care should be taken to note that this does not apply to all widgets, just
this one.
3. The `force_unicode(w) for w in self` on line 21 ought to be
explained. I know what it does because I've read the code for
`RadioFieldRenderer` and know that it has a custom `__iter__` method which
yields `RadioInput` objects, but that's not readily obvious to anyone
else.
4. The new documentation about customizing the radio widget should be
linked from the
[http://docs.djangoproject.com/en/dev/ref/forms/widgets/#django.forms.RadioSelect
RadioSelect widget] docs.
5. Your first example imports `mark_safe` and `StrAndUnicode` which are
not used.
6. On a purely grammatical note, "In particular its renderer function."
[lines 9-10 of your patch] is a sentence fragment.
7. Not that there's anything technically wrong with is, but I might pick
a field name other than `os` for the example simply because `os` is a very
common python module and there's no reason to trigger confusion in
people's minds.
This patch is a good start, and with a little more work will make a
valuable addition to the docs. With a quick turnaround it can even still
make it into the 1.3 milestone.
--
Ticket URL: <http://code.djangoproject.com/ticket/15244#comment:3>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
