-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
fix: fix formactions template to consider arguments #144
base: main
Are you sure you want to change the base?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick response.
Just one small comment about field_class
. I think that needs adding.
<div class="aab {{ label_class }}"></div> | ||
{% endif %} | ||
|
||
<div class="{{ field_class }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need to add field_class
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is FormActions
used to wrap Field
s? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <39445562+smithdc1@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: I wasn't able to run pytest
after following the README. manage.py
is missing. The only way I got it running was by adding it to the root of the project. If this is acceptable I can add it in a separate PR.
<div class="aab {{ label_class }}"></div> | ||
{% endif %} | ||
|
||
<div class="{{ field_class }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is FormActions
used to wrap Field
s? 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? |
@smithdc1 Any chance you could re-review this? |
Thanks for the ping. This looks good to me One final question -- do you think this needs an entry in the changelog? |
Adds
id
andcss_class
to thediv
. Also addsrow
to 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