Bootstrap4 radio & checkbox

[smithdc1]

This PR adds the validation 'is-invalid' classes to the remaining field types (checkbox, checkbox multiple and radios). Fixes #865

With this work I was changing the same lines as PR #892 so I've included those fixes there, along with the updated tests required for this change.

For the inline radios and checkbox the format doesn't look right when the validation fails and the error message is displayed. A bit of research seems to indicate this is a known bug in bootstrap 4 and has been fixed for bootstrap 5. Bootstrap issue

Tests have been updated and are passing and below is the image of the current output.

Edit: Fixed typo on first line.

[codecov-io]

Codecov Report

Merging #897 into master will decrease coverage by 0.5%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #897      +/-   ##
==========================================
- Coverage   94.83%   94.33%   -0.51%     
==========================================
  Files          24       24              
  Lines        2712     2735      +23     
  Branches      246      248       +2     
==========================================
+ Hits         2572     2580       +8     
- Misses         95      110      +15     
  Partials       45       45

Impacted Files	Coverage Δ
crispy_forms/templatetags/crispy_forms_field.py	85.84% <100%> (-9.39%)	⬇️
crispy_forms/helper.py	90.77% <100%> (+0.04%)	⬆️
crispy_forms/tests/test_layout_objects.py	97.15% <100%> (ø)	⬆️
crispy_forms/tests/test_form_helper.py	93.57% <100%> (+0.02%)	⬆️
crispy_forms/tests/test_layout.py	95.62% <100%> (+0.27%)	⬆️
crispy_forms/templatetags/crispy_forms_utils.py	70.37% <0%> (-7.41%)	⬇️
crispy_forms/tests/test_utils.py	94.28% <0%> (-5.72%)	⬇️
crispy_forms/tests/test_tags.py	98.27% <0%> (-0.87%)	⬇️

---

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 2c41dbc...587acb1. Read the comment docs.

[bryan-brancotte]

Hi @smithdc1

Thanks for the PR, it fixes known bug on not rendered error message and use the BS4 template for radio and checkbox.

I've fixed two typo that I found:

• use custom-checkbox instead of custom-radio in multi checkbox css class
• use custom-control custom-checkbox for single checkbox instead of form-check

I am convinced that it is a good idea to use a more modern rendering for these two out of the box. As the css classes changes, crispy user that have already customized the rendering of the checkbox and radio will see there customization be replaced by the BS4 customization, and might not appreciate it. Here is what i get on an other project where I also have customization:

• before
• after

Maybe we should make it enabled by default, but optional from the helper ? So users can easily keep their customization by setting something like:

    helper.use_custom_control = False

If so I can submit later a PR

Best regards

[bryan-brancotte]

In addition, my main concerne about adding a new option in the helper is that I am wondering if user will find it. Also how can we say loud and clear that if you customize the radio and checkbox, the next release will probably break your theme and you should set helper.use_custom_control = False.

@carltongibson what is your opinion on the matter?

[smithdc1]

@bryan-brancotte great idea to improve the styling of the single checkbox so it's consistent with the other elements.

[carltongibson]

Hi @smithdc1 — can I ask you to split the changes from #892, with whatever changes to the tests are needed, into a separate PR? We can review review that quickly and merge, and then rebase this on top of that. (Makes it much easier to see what's going on, both for review and when looking at the history later.)

On the customisation question, how are you doing that? Overriding the template or... ? I'm not sure of the exact context, so unsure how to answer yet...

Thanks both for your input! 🥇

[bryan-brancotte]

Hi @smithdc1 , I took the liberty to remove changes from #892 from this PR as suggested by @carltongibson . We now can focus only on using BS4 theme.

On the optional customisation question, my idea is to mimics how disable_csrf i.e an attribute in the help, passed to the contexte when rendering the template, and in the template something like {%if use_custom_control%}custom-control-input{% else %}form-check-input{% endif %}.

Please find attached a patch applying my proposal. Should I commit it to this PR ? Or make a fork from @smithdc1 PR ?
bs4-customization-optional.zip

[carltongibson]

Hi @smithdc1 @bryan-brancotte.

I merged #892 and rebased this on that. I also squashed to a single commit. Please confirm that I did that correctly.

@bryan-brancotte I think with the docs and tests your suggestion is fine. Push it as a second commit here, and then we can all check that it suits what's needed.

Super work!

[smithdc1]

Both, thanks for your support on this. I'm learning loads from you both.

I'll try and tave a look at this later.

[smithdc1]

Hi I've reviewed this and I've spotted one unrelated bug. The id for the input on radio's is id="id_{{ field.id_for_label } which generates id_id_....(image below) #892 fixed it for the label, but not for the input.

Shall I raise a separate PR?

Other than that it looks good (to me atleast!)

[carltongibson]

...generates id_id_...

@smithdc1 You #899 is addressing this right? Yes, let's follow-up there.

[carltongibson]

@bryan-brancotte Do you want to push you suggestion here? (So that we don't cause an issue for folks that have already customised the default templates?)

[carltongibson]

Otherwise we can take this as it is, and then leave #899 free to clear up any remaining id_ inconsistencies.

(Having two PRs open at the same time on the same line leads to too many rebases...)

[bryan-brancotte]

Hi @carltongibson

Sorry for the delay, I was offline these past days. Here is my commit making the customization optional. I don't know if you prefer to merge or squash and merge, so I will let you choose.

Thanks both for the work, also learning a lot !

[smithdc1]

Hi @bryan-brancotte thanks for your help with this! I think this is all great stuff! I'm looking forward to learning how your new helper attribute and tests work, in the meantime I have a couple of notes.

Error Message
The code for checkboxselectmultiple.html made me think as it wasn't obvious why there was a new <input> tag at the end. However, when I dropped the code it into crispy-test-project I saw that you have come up with a solution for the error message format.

In the screenshot below we can see your failing checkbox looks much better than my failing radio as I was trying to fit the error message inside the last <div>

So, shall we replicate your checkbox solution for for radio as well? Also is it worth adding a comment in the code to explain what it does, although I notice the templates have very little in the way of comments.

Documentation
Earlier on in the notes you mentioned about being concerned if the user will find the new helper use_custom_control. We should add this change to the CHANGELOG.md as a new 'feature' for the next release?

Current output of PR

[smithdc1]

@bryan-brancotte I copied your excellent solution for checkbox error messages to radios.

Is this PR good to merge now?

p.s. enjoyed working on this with you both, it has been a great team effort!

[bryan-brancotte]

Hi @smithdc1 , it look nice, but I am surprise of what I did in this commit 2db5d71#diff-51f77825413a3dd8f78e8254caea94adR20-R22, maybe it is a copy paste error. I have to look into it but this week will be too busy for me.

[carltongibson]

Hi @smithdc1 @bryan-brancotte. I was away last week and more, so I've been quiet here. Where are we? What help do we need to get this in? Sorry, I've lost track slightly — the diff is looking good!

[smithdc1]

Hi Carlton - I hope you had a great time at DjangoCon.

I think we're nearly there with this, just one bit we're unsure of for in-line checkbox and radios.

Bryan came up with a good looking solution to the inline error message issue, it's similar to a comment on the bootstrap GitHub pages (link below) where an additional 100% flexbox is added.

The current solution is a bit strange (but looks great!) as it's in a new div and therefore there is an additional (and lonely) input box to make it work. I did try adding an additional box in the main div but I failed at getting that to work.

So the help we need is to understand if Bryan's solution is ok merge? (Probably the additional input is of most concern?)

My latest commit to the PR is just for this change to radio so it's easy to see what I'm trying to refer to!

(Link to bootstrap recommended solution to the error comment bug)
twbs/bootstrap#26407

Apologies for style, typos etc as on mobile.

[bryan-brancotte]

Hi all

Thanks @smithdc1 for explaining my solution, to be honest it was an attempt to fixe the problem, but I was not fully satisfied with the solution and did not want to commit it. Any way it is now here and as you say it works, and yes it is strange. The idea behind is that BS4 wants to have the error avec an custom-control-input is-invalid, and we can't as we inline-ed them. As the input has no name, it's content will not be send and thus is There may be solution, but it requires to include custom css which, tell me if I am wrong, we don't.

The PR is ok for me if @carltongibson is ok with my strange solution

[carltongibson]

Hi @bryan-brancotte. Could you just comment with the skeleton of the desired/ideal html (just for this fragment here) so I can compare with what we have?

(There's always a way 🙂)

[bryan-brancotte]

What we have without the patch

What we achieve

[carltongibson]

OK both, I think this is super. Very clever really.

1. Can we add a DTL comment saying why the extra input is there. (I know there aren't many of these, but I'd be happy to see them added over time...)
2. The BS custom-control-input already handles the required CSS here right. (Setting opacity:0; on the input elements.) So I think we're OK no? (We could ship some Form.Media, but best not if we can avoid it: the whole point of Bootstrap is that it lets you just do the HTML, I think.)

I'm happy for this to go in if you both are! Thanks for the super team work.

[bryan-brancotte]

Comment added

[bryan-brancotte]

Thanks to both of you

[carltongibson]

Right, good stuff!

Now, towards a short-list for a v1.8 release. (Django 3 compat, plus any other BS4 issues we can clear up.) 🚀

[smithdc1]

Awesome work. Thank you both. 👏