jazzband / django-floppyforms

Full control of form rendering in the templates.
http://django-floppyforms.readthedocs.org/
Other
839 stars 145 forks source link

Make default template name changeable thru settings.py #113

Closed walnutist closed 10 years ago

walnutist commented 10 years ago

Now the default template name for each widget could be changed thru the setting FLOPPYFORMS_DEFAULT_WIDGET_TEMPLATES.

The FLOPPYFORMS_DEFAULT_WIDGET_TEMPLATES is a dict with key as the qualified class name, and value as the template name. The template name could be a callable and it would be called with context and widget itself as parameters during the rendering time.

walnutist commented 10 years ago

If this pull-request is acceptable, I could update the docs accordingly.

gregmuellegger commented 10 years ago

Hi, I appreciate the time and effort that went into this pull request. But unfortunatelly I don't think that this is a good addition in the philosophy of the package.

I do not see where changing the default templatenames provides any additional benefit above just overriding the default templates.

Do you have a real usecase for this, could you explain it a bit?

walnutist commented 10 years ago

Hi Gregor, sorry that I was quite busy in last few days. So here is the case I was thinking of: I want to change the default template for the RadioSelect but not CheckboxInput. The only way to do so by default is to extend the RadioSelect class and override the template, or monkey patching which obviously isn't a good choice. NOTES: As far as I understand. Both RadioSelect and CheckboxInput are not defining any template_name, both of them would fallback to the parent class Input for the template_name retrieving. Correct me if I am wrong.

Of course, you could always do the conditional determination within the base template "floppyforms/input.html" and include different templates, but that feels just not right to me.

gregmuellegger commented 10 years ago

By default, the checkbox widget is using the input.html template. That's right. However you can overwrite this like this:

class MyForm(forms.Form):
    checkbox = forms.BooleanField(widget=forms.CheckboxInput(template_name='checkbox.html'))

I know that's quite verbose but might do it for you for now.

What I'm thinking about as a general fix is, that all widgets should get their very own template that just extends input.html by default and can be overriden if desired. So that you have checkbox.html for default with the contents:

{% extend "floppyforms/input.html" %}

What do you think about this?

walnutist commented 10 years ago

Understand. But I feel that having a configurable setting would make things more consistent and DRY. Putting the determination of template_name into a configurable even a callable function, gives a great degree of freedom to any django developer. The approach you proposed is nice, but it might complicate the template a lot.

E.g., with a callable function to determine the template name, I can do:

if user.is_superuser():
    return "path/to/the/editable/widget/template"
else:
    return "path/to/the/readonly/widget/template"

Of course, the same logic could be embedded into template itself, but it is just too heavy from the template philosophy in my mind.

Certainly, philosophy is always a flavor thing...

gregmuellegger commented 10 years ago

Yes, that is a question of philosophy kind of. And floppyforms philosophy is mostly to give the template author the power for customization.

And introducing a hook that allows for changing the template name in the python code is nothing in reach for the template author. Even if you are just a few people on a project it still makes sense in this mind set to separate the concerns. If the template should change if the user is superuser, than let's put this logic into the template where it belongs. I came across the issue that the widget's might not always have this information readily available (e.g. the login user that comes from the request context processors), but then let's fix this problem instead of working around it with dynamically setting the template name.

Having the logic in the template is just one line more:

{% if user.is_superuser %}
    {% include "path/to/the/editable/widget/template" %}
{% else %}
    {% include "path/to/the/readonly/widget/template" %}
{% endif %}