Opened 8 years ago

Closed 2 years ago

Last modified 14 months ago

#4592 closed Cleanup/optimization (fixed)

Make CheckboxSelectMultiple more like RadioSelect

Reported by: Scott Sinclair Owned by: bmispelon
Component: Forms Version: master
Severity: Normal Keywords: feature
Cc: scott-django@…, michal@…, andy@…, bmispelon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It would be nice if CheckboxSelectMultiple could work the same as RadioSelect does, so that it could provide the ability to render each checkbox/item individually.

This is currently done in RadioSelect by the use of RadioSelectRenderer and RadioItem.
It is presumed that the same can be done for CheckboxSelectMultiple, and would be beneficial, as both of these Widgets, are fundamentally going towards the same goal. One allows multiple choices, the other only allows one.

Attachments (3)

4592-1.diff (9.7 KB) - added by Matt McClanahan <cardinal@…> 8 years ago.
4592-2.diff (9.2 KB) - added by Matt McClanahan <cardinal@…> 8 years ago.
Tweak
4592-3.diff (9.2 KB) - added by Matt McClanahan <cardinal@…> 8 years ago.
Updated for the unicode branch merge

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by Scott Sinclair <scott-django@…>

  • Cc scott-django@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Changed 8 years ago by Matt McClanahan <cardinal@…>

comment:2 Changed 8 years ago by Matt McClanahan <cardinal@…>

  • Has patch set

Changed 8 years ago by Matt McClanahan <cardinal@…>

Tweak

comment:3 Changed 8 years ago by SmileyChris

  • Keywords CheckboxSelectMultiple removed
  • Triage Stage changed from Unreviewed to Ready for checkin

Worthy ticket, looks good - promoting straight to checkin.

See semi-related ticket: #4228. Whichever ticket is committed first, the other one will need to be updated

Changed 8 years ago by Matt McClanahan <cardinal@…>

Updated for the unicode branch merge

comment:4 Changed 8 years ago by Michal Ludvig <michal@…>

  • Cc michal@… added

comment:5 Changed 8 years ago by Michal Ludvig <michal@…>

  • Triage Stage changed from Ready for checkin to Design decision needed

This patch was mentioned as an alternative to #4785, I am however missing something. Should this one be committed, what am I supposed to do to get <div>-wrapped inputs instead of <ul><li> ones? That's the purpose of #4785.

It looks to me that the reference to ChoiceFieldRenderer class that's responsible for rendering the <ul><li> group is hardcoded in RadioSelect and CheckboxSelectMultiple classes. In other words I can't see a way to get <div>-style output with this patch.

Could we perhaps further modify the ChoiceFieldRenderer from the last patch to have the '<ul>%s</ul>' and '<li>%s</li>' in a class variable, similar to what's done in #4785. And also add constructors to RadioSelect and CheckboxSelectMultiple classes with parameter field_renderer defaulting to ChoiceFieldRenderer? That way it would be easily possible to create a subclass of ChoiceFieldRenderer with different HTML tags and push this new renderer to render() method of the two classes.

Should I make such a patch?

comment:6 Changed 8 years ago by SmileyChris

Michal, I guess that I should have said #4228 combined with this one was an alternative to #4785.

comment:7 Changed 8 years ago by ubernostrum

Closed #4785 in favor of this because there's more discussion here; the patch from #4785 is worth looking into for this ticket, though.

comment:8 Changed 7 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:9 Changed 5 years ago by AndrewIngram

  • Cc andy@… added

This ticket looks like it's in limbo but it's something which I am keen to see progress. I've recently run into the need to override the way CheckboxSelectMultiple renders and my solution was pretty hacky. What needs to be done to get this going again? I'd be interested in getting involved, but I'm not sure whether to continue down the route of the existing patches or to look for different solutions (is this part of a bigger problem for allowing easy overrides of how any widget is rendered?).

comment:10 Changed 5 years ago by anonymous

  • Cc scott-django@… michal@… andy@… removed

It Would be nice to access individual items from RadioSelect,CheckboxSelectMultiple.

Anyone working on it ?

comment:11 Changed 5 years ago by SmileyChris

  • Cc scott-django@… michal@… andy@… added

Says mr anonymous, also deleting everyone's CC.

Bringing this up in the django dev group (at 1.3 proposals stage) would be the next step towards progress.

comment:12 Changed 5 years ago by adamnelson

  • Patch needs improvement set

comment:13 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:14 Changed 4 years ago by lupino

  • Easy pickings unset

Furthermore, I'd like to add that the current hard-coded output is not consistent with the output of someform.as_ul, which renders checkboxes like

<li><label for="id_blubb">Blubb:</label> <input id="id_blubb" type="checkbox" name="mobil" value="1" /></li> 

whereas the format of the list elements in CheckboxSelectMultiple is

<li><label for="id_blubb_0"><input type="checkbox" name="blargh" value="1" id="id_blubb_0" /> Blubb </label></li>

comment:15 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:16 follow-up: Changed 2 years ago by bmispelon

  • Cc bmispelon@… added
  • Owner changed from nobody to bmispelon
  • Patch needs improvement unset
  • Status changed from new to assigned

I've brought Matt McClanahan's old patch up to date and added some documentation [1]

The tests pass without modification so this change should be completely backwards-compatible (I was actually surprised at how well it worked).

One thing to note is the disappearance of django.forms.widgets.RadioInput which was not part of the public API (it's not in __all__ and was not documented anywhere).
If needed, it would be possible to re-introduce it as an alias of the new RadioChoiceInput with a deprecationwarning.

In my branch, the commit for this ticket depends on two other ones: #4117 and #19874 which are somewhat related.

[1] https://github.com/bmispelon/django/compare/ticket-4592

comment:17 Changed 2 years ago by bmispelon

Here's the link to the pull request: https://github.com/django/django/pull/1011/files

comment:18 in reply to: ↑ 16 Changed 2 years ago by claudep

Replying to bmispelon:

One thing to note is the disappearance of django.forms.widgets.RadioInput which was not part of the public API (it's not in __all__ and was not documented anywhere).
If needed, it would be possible to re-introduce it as an alias of the new RadioChoiceInput with a deprecationwarning.

Yes, I think that adding a deprecation shim for RadioInput would be a good thing, as it does not cost us much. See http://djangosnippets.org/snippets/2159/ for an example of a RadioInput usage. Apart from that, the patch is RFC.

comment:19 Changed 2 years ago by Claude Paroz <claude@…>

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

In 9ac4dbd7b53d187ca54f28e247d3a120660938ca:

Fixed #4592: Made CheckboxSelectMultiple more like RadioSelect

I refactored RadioSelect and CheckboxSelectMultiple to
make them inherit from a base class, allowing them to share
the behavior of being able to iterate over their subwidgets.

Thanks to Matt McClanahan for the initial patch and to
Claude Paroz for the review.

comment:20 Changed 14 months ago by Tim Graham <timograham@…>

In 797d742662a47417ab875b7a1f5d7919caf5363f:

Removed django.forms.widgets.RadioInput per deprecation timeline.

refs #4592.

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