Add check to handle empty form attributes in general non-crispy template#3140
Add check to handle empty form attributes in general non-crispy template#3140podliashanyk wants to merge 2 commits intomasterfrom
Conversation
If no form.attrs are set then the standard form fields will be rendered. Works as a failsafe.
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports MegaLinter is graciously provided by |
Test results 9 files 9 suites 8m 30s ⏱️ Results for commit 626cc3e. ♻️ This comment has been updated with latest results. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3140 +/- ##
==========================================
- Coverage 60.44% 60.42% -0.02%
==========================================
Files 605 605
Lines 43749 43745 -4
Branches 48 48
==========================================
- Hits 26443 26435 -8
- Misses 17294 17298 +4
Partials 12 12 ☔ View full report in Codecov by Sentry. |
johannaengland
left a comment
There was a problem hiding this comment.
The tests fail since we have not added attrs to the management profile form in #3105.
And generally I thought we were moving away from having the catch-all {{ form }} since we will uncrispify all of nav.
If we don't want to have to add attrs to all forms this tactic is not ideal, since it shows that the tests fail even with the fallback.
It is not necessarily a complete and/or appropriate fallback for forms that are originally non-crispy. This is in ulikely unforeseen cases where for some reason Django widget can not be rendered as a foundation-5/field.html template without loosing some field data. Or a much more likely scenario when styles in foundation-5/field.html are not fitting for the form when reusing this template. To sum it up: it should be generic enough for forms that are originally non-crispy but it is not a given. So if this template is to be reused by non-crispy forms (in contrast to uncrispified forms), the rendering of forms should be properly tested.
1ecddf3 to
626cc3e
Compare
|
johannaengland
left a comment
There was a problem hiding this comment.
Does it make any difference now that {% include 'foundation-5/errors.html' %} will only be rendered if forms.attrs is set? Because before if this template was used even without setting form.attrs it would include that template
Good point. Furthermore upon further inspection of the diff I am actually inclined to close this one without merging. After inspecting Django code, it seems to me that having the original implementation of |
I agree with you on that. Let's close this one |
Originally mentioned in #3105 (review)
@lunkwill42 har suggested IRL to add the check to the general template.
A general failsafe for #2794