makinacorpus / django-leaflet

Use Leaflet in your Django projects
GNU Lesser General Public License v3.0
716 stars 282 forks source link

Leaflet Form Widget inside a django MultiWidget #322

Closed 3j14 closed 3 years ago

3j14 commented 3 years ago

We are currently using the Leaflet widget inside of a django MultiWidget. The problem is that the get_context method of the MultiWidget only passes the widget context of any of the subwidgets to the context:

subwidgets.append(widget.get_context(widget_name, widget_value, widget_attrs)['widget'])

(can be found here)

Thus, crucial parts of the context (e.g. module) will be missing when rendering the widget's template. Basically, all additional context attributes from the LeafletWidget._get_attrs(name, ...) method are missing from the context.

The way we solve this is by replicating the fields from _get_attrs in the get_context method of our custom MultiWidget. However, this is very inconvenient and might break with future updates.

I was wondering if we could either

1) Make _get_attrs public by removing the underline at the beginning of the method name. This way, it would be easy to get all additional context attributes that are required for the widget to work in custom widgets.

2) Put crucial attributes that are required by the widget's template into the widget attribute.

The first option would hopefully introduce the least breaking changes as private methods should not be used much outside of this project. However, there is of course the possibility that someone uses this method in a custom widget that inherits from this widget. In such a case, an alias _get_attrs = get_attrs should be sufficient (?). The later option would probably be the most consistent and cleanest approach. However, it would most definitely introduce more breaking changes.

What do you think? Is this out of the scope of the LeafletWidget? Of course I can implement the changes and submit a pull request but I first wanted to discuss the most sensible approach. Or am I missing something completely?

3j14 commented 3 years ago

It turned out that there are 3 more fields or attributes that are required for the widget to render or work properly:

(module too, but it is overwritten by _get_attrs anyway).

All those attributes are set in the BaseGeometryWidget.get_context method.

Note that all context variables that are not inside the widget field will not be passed when using the MultiWidget class.

Considering this, option 2. from my original comment seems to make less sense, as django itself also does not put these fields inside of the widget field. When I think about it, the widget field is also not be the right location for these attributes. It is generally used for the attributes of an HTML tag (e.g. <input name="{{ widget.name }}">).

Making the _get_attrs method public will still help the cause.

Gagaro commented 3 years ago

Feel free to do a PR with the changes. We should have an alias from _get_attrs to the new method with a deprecation warning.

3j14 commented 3 years ago

Thanks for the response. I'll see when I have the time to prepare the PR.

3j14 commented 3 years ago

Sorry that it took me so long. I finally came around to create a pull request (#326).