Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #13009 -- Added BoundField.widget_type property. #12656

Merged
merged 1 commit into from Apr 10, 2020

Conversation

smithdc1
Copy link
Member

@smithdc1 smithdc1 commented Apr 3, 2020

Ticket

The issue explained in the ticket of rendering fields (widgets, really) dependant upon their type is a key part of crispy-forms. This project solved it by adding a limited number of filters so you can do something like this.

{% for field in fields %}
{% if field|is_checkbox %}
  # render a checkbox
{% elif field|is_file %}
  # render a file field
[...]

I've replicated that approach here, and for completeness I've added a filter for each of the widget types, added tests and docs.

There are comments on the ticket about multifield (multiwidget). I think for now just knowing it is a multiwidget is enough. Over at crispy-forms the project still hasn't found a way to deal with these widgets and all their complexities.

Appreciate your thoughts on this one.

@claudep
Copy link
Member

claudep commented Apr 4, 2020

I'm not so much thrilled by this new load of new template filters. What about a new BoundField property (widget_type?) returning the lowercased widget class name stripped from the widget or input strings?
In that case, if I have defined a MyCustomWidget, the (bound)field.widget_type would by mycustom.

@smithdc1
Copy link
Member Author

smithdc1 commented Apr 4, 2020

I've updated this to add a new property to BoundField as suggested. Here are what I think the pros/cons of each option are.

I think the filter approach gives a better result in the templates. i.e. I think that 1) is better than 2)

  1. field|is_checkbox
  2. `field.widget_type == 'checkboxinput'

However, from a Django perspective the second option is by far a cleaner implementation and has the benefit of working with custom widgets. The filter method; however is, well, a little repetitive.

So for me, it's a trade off between a better experience for users building their templates vs impact on Django code base.

On the current version, the tests need moving to a better home, but I'm not quite sure where they are best placed. Appreciate some guidance on this.

@felixxm
Copy link
Member

felixxm commented Apr 7, 2020

@smithdc1 I agree with Claude. I would chop new (one-line) filters and limit this change to the new widget_type property.

django/forms/boundfield.py Outdated Show resolved Hide resolved
django/forms/boundfield.py Outdated Show resolved Hide resolved
docs/ref/forms/api.txt Outdated Show resolved Hide resolved
django/forms/boundfield.py Outdated Show resolved Hide resolved
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I squashed, rebased, and pushed a fixup with some suggested edits.

  • Removed the docstring from widget_type. I didn't think the full example appropriate there, and by the time I'd removed that, it just repeated the single code line, so...
  • Moved the tests with the other boundfield tests in test_forms.
  • Some small rewording edits.

This LGTM. 👍 Thanks @smithdc1

Copy link
Member

@claudep claudep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@felixxm felixxm changed the title Fixes #13009 -- provide django.forms field type info for use in templates Fixed #13009 -- Added BoundField.widget_type property. Apr 10, 2020
@felixxm
Copy link
Member

felixxm commented Apr 10, 2020

Thanks y'all 👍

@felixxm felixxm merged commit a350bfa into django:master Apr 10, 2020
@smithdc1 smithdc1 deleted the 13009 branch April 10, 2020 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants