Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32541 closed Cleanup/optimization (duplicate)

Separate context and rendering in forms

Reported by: Dylan Verheul Owned by: nobody
Component: Forms Version: 3.1
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I've been working on packages that generate HTML based on Django forms for a while. I find myself duplicating certain parts of Django code, because there is no clear separation of the context (as in template context) that is being generated, and the HTML output. A small example of this is the code that renders the HTML for a label.

https://github.com/django/django/blob/76c0b32f826469320c59709d31e2f2126dd7c505/django/forms/boundfield.py#L133

This code takes arguments for and then generates contents (based on actual contents and a suffix) and attrs and then generates HTML. It doe snot always generate a label tag, which I find weird but is besides the point of this issue.

My suggestion would be to split these kind of functions into a context generator

def get_label_tag_context(self, contents=None, attrs=None, label_suffix=None):
    ...
    return {
        "contents": ...,
        "suffix": ...,
        "attrs": ...,
    }

And the actual rendering of the tag

def get_label_tag(self, contents=None, attrs=None, label_suffix=None):
    context = self.get_label_tag_context(contents=contents, attrs=attrs, label_suffix=label_suffix)
    attrs = flatatt(context["attrs"]) if context["attrs"] else ""
    return format_html('<label{}>{}</label>', attrs, context["contents"])

Now, if I want to write my own label renderer, I can get the exact same context that Django uses.

For widgets, I would like to see the same solution. The context for a widget is now part of the rendering process, and contains code that cannot be rached unless you use Django rendering. If you want to apply different rendering to a widget on a per widget basis, you have to duplicate code or hack the generated HTML.

https://github.com/django/django/blob/76c0b32f826469320c59709d31e2f2126dd7c505/django/forms/boundfield.py#L80

Here, a split in generating context and rendering the context would also help third party form packages to work with the same data Django has.

I'm creating this ticket to get feedback on the idea, and to see if a PR separating context and rendering in the forms section of Django would be welcome. I'd be willing to work on this.

Change History (5)

comment:1 by Chris Jerdonek, 3 years ago

I'm creating this ticket to get feedback on the idea, and to see if a PR separating context and rendering in the forms section of Django would be welcome. I'd be willing to work on this.

It's probably better to ask for feedback on the Django developers list:
https://docs.djangoproject.com/en/dev/internals/mailing-lists/#django-developers
https://groups.google.com/g/django-developers

comment:2 by David Smith, 3 years ago

I think we need to be mindful of ticket #31026 and the associated pr which moves forms to a template render. For items such as labels they will be able to be customised via overriding a temple, which could be enough here?

For widgets, there is already the ability to over ride templates, and if you want to go futher I find that a boundfield's subwidgets usually has the information needed to render each widget. For example, it can include selected and built attrs. It's not clear from your ticket what extra context you would need that is only available via django rendering?

Here's an example of the information available in a subwidget.

│ choice_label = 1
│ data = {
│ 'name': 'checkbox_select_multiple',
│ 'value': 1,
│ 'label': 1,
│ 'selected': True,
│ 'index': '0',
│ 'attrs': {'id': 'id_checkbox_select_multiple_0', 'checked': True},
│ 'type': 'checkbox',
│ 'template_name': 'django/forms/widgets/checkbox_option.html',
│ 'wrap_label': True
│ }
│ id_for_label = 'id_checkbox_select_multiple_0'

comment:3 by Dylan Verheul, 3 years ago

Resolution: duplicate
Status: newclosed

Thank you for pointing out #31026. This seems to at least partly resolve the problem I describe, and it would be better to sit that one out before taking any further action.

comment:4 by Carlton Gibson, 3 years ago

Thanks both.

I think we need to be mindful of ticket #31026...

Exactly right.

I loathe to say when anything might happen but, #31026 is ≈top of my long-list for 4.0 so … 😀 — if you follow there Dylan you can see what remains once we merge that.

comment:5 by Dylan Verheul, 3 years ago

👍 Thanks both!

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