Improve multivalue support

[smithdc1]

This is some early work towards fixing #831 . This isn't much more than a proof of concept as there are many more things to consider, along with tests, documentation etc.

However, I'd appreciate views on if this is a reasonable approach to this problem and if it is worth exploring further.

A few notes:

1. field.html has been updated to use multifield.html

2. a new template tag has been created. Most of it is copied from here with a few modifications to get it working for my requirements. This tag allows us to loop over each field within a multifield.

3. multifield.html iterates over the multifield allowing us to put each <input> within a <div> allowing us to add a col class for bs4 style.

[smithdc1]

I've had a few thoughts on this over the weekend.

There is duplication in render_multi_field and crispy_field, particularly the way that the additional attributes are handled. They also do this in different ways, we should at least adopt the crispy_field way of doing things, I think. I feel consistency is most important.

Do we add the additional 'looping' functionality to crispy_field - maybe through an additional attribute which we can catch and do something with. How do we do this in a way which is backwards compatible.

Multifield.html needs to be able to render different code depending what field is within the multifield. Most may be straight forward textinput but if there is a radio or checkbox then additional divs are needed and the css on <input> is different also.

This will be a fairly big change to the multifiled style, so should we need to add a flag for backwards compatibility.

[smithdc1]

I've taken the approach to base MultiFieldAttributeNode on much of the CrispyFieldNode with the added functionality to render the individual field that is passed into the function. This means there is a fair amount of duplication, but style of passing in attributes is consistent and should be easier to merge the two functions if we decide that is ok.

I've also stripped back the additional CSS styles in this function, we should pass it in via the template. This is a slightly wider point which I'll come back to post 1.9.0. I'd like to make some more progress on using the template packs to pass in CSS styles rather than have them coded into the core application. This will make it easier to decouple the template packs.

[smithdc1]

I've made some more progress on this, and I've learnt that 'mulivalue' and 'multifield' has caused me quite some confusion. I'll delete the previous comment as that turned out to be completely wrong!

What I've been trying to solve here is 'multivalue' fields. e.g. SplitDateTimeField and this is completely different to the multfield layout object in crispy-forms.

I've therefore created a new template for 'multivalue' fields. In addition to what I see as the standard usage of multivalue fields I've also added some support for a single checkbox. However, I've not gone as far as allowing multiple checkbox / radio within a multivalue field.

Whilst I've got a couple of remaining tests to fix, here are some images of what it currently looks like.

[codecov-io]

Codecov Report

Merging #976 into master will increase coverage by 0.9%.
The diff coverage is 81.69%.

@@            Coverage Diff            @@
##           master     #976     +/-   ##
=========================================
+ Coverage   95.62%   96.53%   +0.9%     
=========================================
  Files          24       24             
  Lines        2768     2797     +29     
  Branches      244      255     +11     
=========================================
+ Hits         2647     2700     +53     
+ Misses         79       50     -29     
- Partials       42       47      +5

Impacted Files	Coverage Δ
crispy_forms/tests/test_layout_objects.py	97.89% <ø> (+0.68%)	⬆️
crispy_forms/tests/test_form_helper.py	99.48% <100%> (+0.33%)	⬆️
crispy_forms/templatetags/crispy_forms_field.py	92.72% <100%> (+10.48%)	⬆️
crispy_forms/templatetags/crispy_forms_tags.py	91.25% <80%> (-6.21%)	⬇️
crispy_forms/compatibility.py	100% <0%> (ø)	⬆️
crispy_forms/helper.py	94.21% <0%> (+0.94%)	⬆️
crispy_forms/tests/test_utils.py	96.96% <0%> (+2.85%)	⬆️
crispy_forms/tests/test_layout.py	99.68% <0%> (+3.8%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d355c9...28fba7d. Read the comment docs.

[smithdc1]

Hi @carltongibson please could I have some guidance.

I'm struggling to understand what is happening with this test.

django-crispy-forms/crispy_forms/tests/test_layout_objects.py

Line 55 in 77f4ea7

assert html.count('rel="test_timeinput"') == 2

The html being produced currently is as per below (bs3 template pack). There are two input fields being produced for the splithiddendatetimewidget and therefore three inputs in total for a SplitDateTimeField. (i.e. a date field, a hidden date field and a hidden time field)

Do you have a view on what the expected behaviour is here?

<form method="post">
    <div id="div_id_datetime_field" class="form-group">
        <label for="id_datetime_field_0" class="control-label  requiredField">
            date time<span class="asteriskField">*</span> </label>
        <div class="controls ">
            <input type="text" name="datetime_field_0" rel="test_dateinput" class="dateinput" required id="id_datetime_field_0">
            <input type="hidden" name="datetime_field_1_0" rel="test_timeinput" style="width: 30px;" class="splithiddendatetimewidget" required id="id_datetime_field_1_0">
            <input type="hidden" name="datetime_field_1_1" rel="test_timeinput" style="width: 30px;" class="splithiddendatetimewidget" required id="id_datetime_field_1_1"> </div>
    </div>
</form>

[sbutler]

I was thinking very long and hard about this problem, and started to wonder why in crispy_forms.templatetags.crispy_forms_field.CrispyFieldNode#render you loop over the widgets, but the test for checkboxes is on the field. Wouldn't it solve all the problems if the checkbox test is just on the widget itself?

I did a proof-of-concept code that does two things:

1. Modifies the various is_XXX tests to accept any of BoundField, Field, or Widget.
2. Changes the loop's tests so that it is on the current widget and not the field as a whole.

I'm still working on it, but for me the initial results here are what I'd want and I think it solves the issue of adding 'form-control' to widgets.

[smithdc1]

Hi @sbutler Thank you for thinking about this. 👍

I also have some thoughts on the piece of code you are referring too (where we add form-control and is-invalid). One of the aims we have as a project is to decouple the template packs and the core logic. This is one area where the two are tightly combined.

I'd therefore prefer we removed this logic from crispy_forms.templatetags.crispy_forms_field.CrispyFieldNode#render and add it into the template packs. We can do this by passing css_class attributes into crispy_field from each template.

Appreciate your thoughts.

[smithdc1]

Note to self.

I wonder if we can loop to a sub template for each sub field rather than trying to fix this in python code.