-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add contrib mixin for Foundation #2
Add contrib mixin for Foundation #2
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 5 6 +1
Lines 176 205 +29
Branches 33 38 +5
=====================================
+ Hits 176 205 +29
Continue to review full report at Codecov.
|
Hi! Thank you for the PR! I just had a quick look and think we can get this merged. Please give me some extra time to have a deeper look. I'll get back to this PR asap. |
Hi Stephan, no problem, take your time! Could you also have a deep look into jeromelebleu#1 please? |
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.
Great work! Thank you!
I added some comments, all of them are just improvements, no real issues but I hope to get it optimized and merged asap. Really cool!
tapeforms/contrib/foundation.py
Outdated
template_name = super().get_field_template(bound_field, template_name) | ||
|
||
if (template_name == self.field_template and | ||
bound_field.field.widget.__class__ in ( |
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 could optimize this further and catch also subclasses of RadioSelect and CheckboxSelectMultiple by replacing the check with something like
if template_name == self.field_template and isinstance(bound_field.field.widget, (forms.RadioSelect, forms.CheckboxSelectMultiple)):
What do you think?
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.
It seems better, indeed, I will change that!
|
||
if field_name in self.fields: | ||
widget = self.fields[field_name].widget | ||
widget.attrs['aria-invalid'] = 'true' |
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.
Are aria-labels a thing we could add to the base mixin to ensure all kinds of form templates we offer support these?
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.
Yes, I think it could be better regarding MDN documentation about aria-invalid
- see here. I will integrate that in the base mixin.
In fact, I dared not change django-tapeforms' behaviors too much, so your comments are really welcome!
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.
Done with jeromelebleu@0b966f2!
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.
Would you mind submitting a PR for that too?
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 have creating a new one with #4, thanks!
@@ -0,0 +1,25 @@ | |||
<fieldset class="{{ container_css_class }} {{ container_css_class }}-{{ field_name }} form-widget-{{ widget_class_name }}{% if required %} is-required{% endif %}{% if errors %} has-errors{% endif %}{% if field.css_classes %} {{ field.css_classes }}{% endif %}"> |
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.
Basically, this template differs from the fields/foundation.html in two points:
- DOM Element used a the container (div vs. fieldset)
- DOM Element used for the text (label vs. legend)
For the label I'm fine with the change but I'm thinking about having a template variable to replace the "container html element" and then extending the field context in the foundation mixin.
Something like <{{ container_element|default:'div' }} ......
. + In the mixin, we could add a context 'container_element': 'fieldset'
(in get_field_context
)
This would help us remove the extra template or at least reducing the compexity of the template (just overide the label block vs. the whole template).
I'm keen on your optinion on that! :-)
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 agree too, it could be great to not have this extra template. I will try to purpose something as you suggest, but maybe in another PR?
All your improvements have been added - at least... - except the one for the extra template which could be done separately! |
This adds a new mixin
contrib.foundation.FoundationTapeformMixin
to support Foundation for Sites. A special case is made for multiple inputs - they are displayed in a<fieldset>
- in order to follow the Form documentation.