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

Improve HTML form rendering #4122

Closed
wants to merge 3 commits into from

Conversation

arkadini
Copy link

Description

This is an alternative approach to handling bound field values during HTML form rendering. Instead of forcing field values to string representations early through .as_form_field() (done in 6b08e97) it makes both native and string representations available to form renderer / templates.

As mentioned in the commit log, it should fix at least #4120 and #4121; effectively it reverts 6b08e97 but should also handle the cases covered in that commit, at least to a degree provable with the test suite.

I also tried adding some tests for issues I could quickly identify as related (e.g. #3139), as otherwise just plain getting rid of .as_form_field(), surprisingly, didn't make any test to start failing. But I'm sure there are more that I have missed.

arkadini added 3 commits May 15, 2016 15:38
Let each form field's HTML template decide if it needs native or
string-ified field value, instead of forcing all templates to always
work only with unnecessarily restrictive string representation.

Fixes encode#4120, encode#4121
@tomchristie
Copy link
Member

On first scan through, the footprint of this change is bigger that I'd like to see. Any ways round we can resolve the choice field issues you mention in a way that doesn't impact the other templates?

@arkadini
Copy link
Author

The essence of the rendering issue with MultipleChoiceField is that deep in the template it ends up doing if "1" in "{12}": ..., so the availability of the underlying set-typed value seems important to fixing it in a reasonably clean way.

So, another approach would be to not remove .as_form_field() but to make sure that the underlying basic/primitive value is exposed on the cloned stringified BoundFields and only check for those types of values in the templates that really need it.

@tomchristie
Copy link
Member

Closed via #4374 - thanks for your input on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants