fix: fix formactions template to consider arguments #144
Conversation
|
@smithdc1 could this please be merged and a bugfix version be released soon? |
|
Hi All, I'd like to see a small test added as that seems to be missing at the moment. A minimal layout could be created with the output asserted something like this. |
|
@JanMalte -- do you have time to add a test for this? |
|
Sorry for the delay. I agree that tests would be good so I finally added some. |
| <div class="aab {{ label_class }}"></div> | ||
| {% endif %} | ||
|
|
||
| <div class="{{ field_class }}"> |
There was a problem hiding this comment.
I think we still need to add field_class.
There was a problem hiding this comment.
I applied all the suggestions but then thought after that field_class might not be appropriate here: Would field_class not be applied to each child field of FormActions?
Consider the following test:
def test_formactions_field_class(self):
test_form = SampleForm()
test_form.helper = FormHelper()
test_form.helper.form_tag = False
test_form.helper.field_class = "field-class"
test_form.helper.layout = Layout(
FormActions(
Field('email'),
),
)
print(render_crispy_form(test_form))Output:
<div
class="mb-3 field-class"
> <div id="div_id_email" class="mb-3"> <label for="id_email" class="form-label requiredField">
email<span class="asteriskField">*</span> </label> <div class="field-class"> <input type="text" name="email" maxlength="30" class="textinput textInput inputtext form-control" required id="id_email"> <div id="hint_id_email" class="form-text">Insert your email</div> </div> </div> </div>As per field.html the <input> gets wrapped into a div with CSS class field-class.
There was a problem hiding this comment.
Is FormActions used to wrap Fields? The crispy-form docs say...
FormActions: It wraps fields in a
. It is usually used to wrap form’s buttons.
With an example form as per the docs. (please forgive the lack of style on the cancel button).
Main branch:
This patch:
This therefore looks like a visual regression to me.
There was a problem hiding this comment.
Thanks! I reverted that change and also brought back the div for label_class to be consistent with the other template packs.
Co-authored-by: David Smith <[email protected]>
| <div class="aab {{ label_class }}"></div> | ||
| {% endif %} | ||
|
|
||
| <div class="{{ field_class }}"> |
There was a problem hiding this comment.
I applied all the suggestions but then thought after that field_class might not be appropriate here: Would field_class not be applied to each child field of FormActions?
Consider the following test:
def test_formactions_field_class(self):
test_form = SampleForm()
test_form.helper = FormHelper()
test_form.helper.form_tag = False
test_form.helper.field_class = "field-class"
test_form.helper.layout = Layout(
FormActions(
Field('email'),
),
)
print(render_crispy_form(test_form))Output:
<div
class="mb-3 field-class"
> <div id="div_id_email" class="mb-3"> <label for="id_email" class="form-label requiredField">
email<span class="asteriskField">*</span> </label> <div class="field-class"> <input type="text" name="email" maxlength="30" class="textinput textInput inputtext form-control" required id="id_email"> <div id="hint_id_email" class="form-text">Insert your email</div> </div> </div> </div>As per field.html the <input> gets wrapped into a div with CSS class field-class.
|
@smithdc1 Brought this PR up to date with main. Could you please re-review? |
There was a problem hiding this comment.
Hi @mschoettle
Thanks for coming back to this! 🎁
I've reviewed again and have added some images of what this looks like. I'm afraid I still think we need to keep field_class. Appreciate your thoughts.
| <div class="aab {{ label_class }}"></div> | ||
| {% endif %} | ||
|
|
||
| <div class="{{ field_class }}"> |
There was a problem hiding this comment.
Is FormActions used to wrap Fields? The crispy-form docs say...
FormActions: It wraps fields in a
. It is usually used to wrap form’s buttons.
With an example form as per the docs. (please forgive the lack of style on the cancel button).
Main branch:
This patch:
This therefore looks like a visual regression to me.
|
@smithdc1 I addressed the requested changes. Could you please look at this again? |
Adds
idandcss_classto thediv. Also addsrowto the class if the form is supposed to be horizontal.I removed the inner divs since those seemed to be not used. Let me know if that is incorrect.
Also, let me know if I should add tests for this.
Fixes #143