Code

Opened 3 years ago

Closed 3 days ago

#17301 closed New feature (duplicate)

Defining fieldsets in a form class

Reported by: msiedlarek Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords: form, forms, fieldsets
Cc: msiedlarek, riccardo.magliocchetti@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Motivation

For a large form grouping of fields is not just the pretty-rendering and template-related issue and I believe there should be way to logically group fields a form class definition. It would also be convenient to have Django automatically render those group of fields, just as easy as it is now to do {{ form.as_p }}.

Idea

The idea is to use existing and well-known conventions. Example:

class PersonForm(forms.Form):
    home_phone = CharField()
    cell_phone = CharField()
    first_name = CharField()
    last_name = CharField()

    class Meta:
        fieldsets = (
            (None, {
                'fields': ('first_name', 'last_name'),
            }),
            ("Phone numbers", {
                'fields': ('cell_phone', 'home_phone'),
            }),
        )

Note usage of conventions already known to many users -- Meta class for options (used in models and ModelForm already) and fieldsets tuple, just like one in ModelAdmin options.

The idea is also not to break anything already working meaning being backward compatible. Forms without fieldsets explicitly defined by user should render normally.

Having a form defined as above user can easily use it in a template. Example:

<form action="" method="post">
    {{ form.as_table }}
    <input type="submit" value="Send" />
</form>

Renders to:

<form action="" method="post">
    <fieldset>
        <table>
            <tr>
                <th><label for="id_first_name">First name:</label></th>
                <td><input type="text" name="first_name" id="id_first_name" /></td>
            </tr>
            <tr>
                <th><label for="id_last_name">Last name:</label></th>
                <td><input type="text" name="last_name" id="id_last_name" /></td>
            </tr>
        </table>
    </fieldset>
    <fieldset>
        <legend>Phone numbers</legend>
        <table>
            <tr>
                <th><label for="id_cell_phone">Cell phone:</label></th>
                <td><input type="text" name="cell_phone" id="id_cell_phone" /></td>
            </tr>
            <tr>
                <th><label for="id_home_phone">Home phone:</label></th>
                <td><input type="text" name="home_phone" id="id_home_phone" /></td>
            </tr>
        </table>
    </fieldset>
    <input type="submit" value="Send" />
</form>

Which I believe is a good default behavior. Other examples:

<form action="" method="post">
    {% for fieldset in form.fieldsets %}
        <fieldset>
            {{ fieldset.legend_tag }}
            <table>
                {{ fieldset.as_table }}
            </table>
        </fieldset>
    {% endfor %}
    <input type="submit" value="Send" />
</form>
<form action="" method="post">
    {% for fieldset in form.fieldsets %}
        <fieldset>
            {{ fieldset.legend_tag }}
            <ul>
                {% for field in fieldset %}
                    <li>
                        {{ field.label_tag }}
                        {{ field }}
                    </li>
                {% endfor %}
            </ul>
        </fieldset>
    {% endfor %}
    <input type="submit" value="Send" />
</form>
<form action="" method="post">
    {% for fieldset in form.fieldsets %}
        <fieldset>
            {{ fieldset.legend_tag }}
            <ul>
                {% for field in fieldset.visible_fields %}
                    <li>
                        {{ field.label_tag }}
                        {{ field }}
                    </li>
                {% endfor %}
                <li class="super-hidden">
                    {% for field in fieldset.hidden_fields %}
                        {{ field }}
                    {% endfor %}
                </li>
            </ul>
        </fieldset>
    {% endfor %}
    <input type="submit" value="Send" />
</form>

Implementation

Yeah, there's a patch. Short description of implementation for those afraid of reading the diff:

  1. Creating BaseFormOptions class, similar to this already present in ModelForm class (for defining model for form). ModelFormOptions now subclasses it. BaseFormMetaclass now handles options-related stuff. (note that this behavior may be used in future for other form options)
  2. Separating all rendering from Form class to a new one - Fieldset. Form not having any fieldsets defined uses the single, dummy fieldset for all its fields.
  3. No editing any of already existing tests. Everything should be backward compatible.
  4. Providing some tests for new behavior - form options and fieldsets.
  5. Providing some documentation. Unfortunately my English skills are, as you see, not that impressive, so it definitely need some touch. Now it's more like set of examples.

Patch

Patch is attached. You can read it on Trac here or on my Django github fork: https://github.com/mikoskay/django/compare/master...form-fieldsets

Attachments (3)

ticket17301-version1.diff (33.2 KB) - added by msiedlarek 3 years ago.
Patch version 1
ticket17301-r17463.diff (33.1 KB) - added by msiedlarek 2 years ago.
Patch updated for r17463 trunk
ticket17301_v2.diff (37.4 KB) - added by lrekucki 2 years ago.
Patch with better code reuse in metaclass.

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by msiedlarek

Patch version 1

comment:1 Changed 3 years ago by msiedlarek

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

TODO

There's one more feature I'd like to add - classes fieldset option, similar to this in ModelAdmin.
But I pre-wrote enough already - give me some +1s for encouragement. :)

comment:2 Changed 3 years ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

Just a quick question: Would it be possible to design this in a way where you have something more generic which Fieldset bases upon? I just ask this because it might be nice to have TabGroups for tabular forms (where each tab could perhaps have fieldsets...). A possible idea would be that forms can have FieldGroups, which can have fields or more FieldGroups. Fieldset would be a special case of a FieldGroup.

I don't know if implementing the feature in more generic terms is easy or not. If it would make this overly complicated, feel free to ignore this comment.

I am marking this accepted based on this django-developers thread.

comment:3 Changed 3 years ago by anonymous

  • Patch needs improvement set

This is actually a great idea! It's possible in current state of the patch by subclassing both Form and Fieldset, but it probably can be made easier for developer. I'll certainly think about it.

Changed 2 years ago by msiedlarek

Patch updated for r17463 trunk

comment:4 Changed 2 years ago by msiedlarek

  • Needs documentation set
  • Patch needs improvement unset

Given some thought on Carl Meyer's voice in fieldsets discussion (https://groups.google.com/forum/?_escaped_fragment_=msg/django-developers/dCQAmD1BBzY/tMesCJk96qEJ#!msg/django-developers/dCQAmD1BBzY/tMesCJk96qEJ) I've decided not to implement any presentation-layer features into basic form fieldsets and leave it as a feature for logical organization of fields. Note, that fieldsets' classes, or using tags other than <fieldset> can easily be achieved using templates and I believe that's where those features belong.

Patch can be considered checkin-ready now (including tests). There's only one problem - documentation desperately needs touch of some better English speaker than myself.

comment:5 Changed 2 years ago by AndrewIngram

Could you make modelform_factory accept fieldsets as an optional parameter, much like fields and excludes? It likes like it would be a pretty small addition but it means the admin site could leverage the same functionality quite easily.

Changed 2 years ago by lrekucki

Patch with better code reuse in metaclass.

comment:6 Changed 2 years ago by lrekucki

I found two odd things in your patch:

One was partially already in Django:

try:
    parents = [b for b in bases if issubclass(b, ModelForm)]
except NameError:
    # We are defining ModelForm itself.
    parents = None

Your patch changes ModelForm to BaseModelForm which makes the "try: except:" look silly, because it will never trigger. It's also now duplicated in BaseFormMetaclass.

Second, is the doubled call to BaseFormOptions.__init__.

I fixed the second by adding make_options method on the metaclass, which can be then overriden, and the first by using isinstance(b, cls) instead of issubclass. This has a small caveat, that it will consider any class using this metaclass as a parent of the form (not just subclasses of BaseForm/BaseModelForm), but I'm pretty sure that anyone doing this, really knows the implications.

comment:7 Changed 2 years ago by msiedlarek

I was actually aware of the second problem, but somehow no good solution came to my mind. What you say sounds good, thanks!

comment:8 Changed 5 months ago by rm_

  • Cc riccardo.magliocchetti@… added

comment:9 Changed 3 days ago by bpeschier

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

Closing as a duplicate of #6630, which has more discussion and a similar approach

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.