django-crispy-forms / crispy-bootstrap4

Bootstrap 4 template pack for django-crispy-forms
MIT License
11 stars 5 forks source link

Fixes checkboxselectmultiple and radioselect repeats form's attributes #16 #17

Closed smithdc1 closed 1 year ago

smithdc1 commented 1 year ago

Fixes #16

smithdc1 commented 1 year ago

@bblanchon & @glenvaughan does this work for you?

bblanchon commented 1 year ago

Thanks, @smithdc1; this change does fix #16. However, it introduces a regression: we can no longer add custom attributes to this <div>.

For example:

from django import forms

class DemoForm(forms.Form):
    language = forms.MultipleChoiceField(
        choices=[
            ("cpp", "C++"),
            ("py", "Python"),
            ("js", "JavaScript"),
        ],
        widget=forms.CheckboxSelectMultiple(),
    )

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.helper = FormHelper()
        self.helper.layout = Layout(
            Field("language", style="background: grey"),
        )

Actual vs expected output:

<form method="post">
  <div id="div_id_language" class="form-group">
    <label>Language</label>
-    <div>
+    <div style="background: grey">
     <div class="custom-control custom-checkbox">
       <input type="checkbox" class="custom-control-input" name="language" value="cpp" id="id_language_0">
       <label class="custom-control-label" for="id_language_0">C++</label>
     </div>
     <div class="custom-control custom-checkbox">
       <input type="checkbox" class="custom-control-input" name="language" value="py" id="id_language_1">
       <label class="custom-control-label" for="id_language_1">Python</label>
     </div>
     <div class="custom-control custom-checkbox">
       <input type="checkbox" class="custom-control-input" name="language" value="js" id="id_language_2">
       <label class="custom-control-label" for="id_language_2">JavaScript</label>
     </div>
   </div>
  </div>
</form>
smithdc1 commented 1 year ago

In your example, I'm surprised that style="background: grey" doesn't also feature on the <input>?

<form method="post">
    <div id="div_id_language" class="form-group">
        <label class="requiredField"> Language<span class="asteriskField">*</span> </label>
        <div style="background: grey">
            <div class="custom-control custom-checkbox">
                <input type="checkbox" class="custom-control-input" name="language" value="cpp" style="background: grey" id="id_language_0" /> <label class="custom-control-label" for="id_language_0"> C++ </label>
            </div>
            <div class="custom-control custom-checkbox">
                <input type="checkbox" class="custom-control-input" name="language" value="py" style="background: grey" id="id_language_1" /> <label class="custom-control-label" for="id_language_1"> Python </label>
            </div>
            <div class="custom-control custom-checkbox">
                <input type="checkbox" class="custom-control-input" name="language" value="js" style="background: grey" id="id_language_2" /> <label class="custom-control-label" for="id_language_2"> JavaScript </label>
            </div>
        </div>
    </div>
</form>

I expect that additional kwargs for Field are only added to widgets / <input>.

bblanchon commented 1 year ago

You're right. I didn't notice, but these attributes also apply to the checkboxes.

But is this the correct behavior, because it seems inconsistent with the other controls? For example, field attributes apply to the <select multiple> element, not each <option>. Why would checkbox-select-multiple behave differently since it's supposed to be a drop-in replacement for <select multiple>?

smithdc1 commented 1 year ago

Interesting question. I always like to ask WWDJ (what would Django do) in cases like this. It's likely that crispy-forms is using django's own templates at this level.

I think this is the expected behavior. Looking at bootstrap's docs it's also the <select multiple> and <input> tags that are targeted for classes rather than <options>. It seems to me that styling <options> isn't that well supported. See this 12 year old ticket for django/django Ticket #16149.

Form Code

``` python class DemoForm(forms.Form): language = forms.MultipleChoiceField( choices=[ ("cpp", "C++"), ("py", "Python"), ("js", "JavaScript"), ], widget=forms.SelectMultiple(attrs={"style": "background: grey"}), ) language2 = forms.MultipleChoiceField( choices=[ ("cpp", "C++"), ("py", "Python"), ("js", "JavaScript"), ], widget=forms.CheckboxSelectMultiple(attrs={"style": "background: grey"}), ) form = DemoForm() form.helper = FormHelper() form.helper.layout = Layout( "language", "language2" ) ```

HTML Output

``` html
```

I wonder if we're going a bit off track -- what's your requirement here, is it to add css classes to this<div> in the template?

bblanchon commented 1 year ago

I understand; it makes sense to follow Django's behavior.

My goal is to add classes form-control h-auto to this <div> so that it looks like other controls and a max-height style because it's getting too tall. But that's not a problem; I'll override this template in my project.