Skip to content

Fixed #17922 -- Added required_css_class to form label #2390

Closed
wants to merge 1 commit into from

3 participants

@coder9042

As suggested in the ticket, I have done what was required.
One test has been corrected, and another has been added.

Related: https://code.djangoproject.com/ticket/17922

@coder9042

If the patch is fine, then I will update the docs and also release-notes(if needed)

@apollo13 apollo13 and 1 other commented on an outdated diff Mar 4, 2014
django/forms/forms.py
@@ -617,6 +617,9 @@ def label_tag(self, contents=None, attrs=None, label_suffix=None):
id_for_label = widget.id_for_label(id_)
if id_for_label:
attrs = dict(attrs or {}, **{'for': id_for_label})
+ if ((attrs is None or 'class' not in attrs) and self.field.required and hasattr(self.form, 'required_css_class')):
@apollo13
Django member
apollo13 added a note Mar 4, 2014

Please try to stay near 80 char per line…

@coder9042
coder9042 added a note Mar 4, 2014

@apollo13
I will do that.
Is everything else fine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coder9042

@apollo13
Well from our talk on IRC, I think you are mistaking this to be affecting functionality.
This patch only improves the look by making use of required_css_class provided. It will add this class to the label only if field has required=True or as by default this is also set to True.
If required= False then label class will not be affected.

@timgraham timgraham commented on an outdated diff Mar 7, 2014
django/forms/forms.py
@@ -617,6 +617,10 @@ def label_tag(self, contents=None, attrs=None, label_suffix=None):
id_for_label = widget.id_for_label(id_)
if id_for_label:
attrs = dict(attrs or {}, **{'for': id_for_label})
+ if ((attrs is None or 'class' not in attrs) and self.field.required
@timgraham
Django member
timgraham added a note Mar 7, 2014

If 'class' is already in attrs, this should append self.form.required_css_class to it, not leave the value unaffected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coder9042

@timgraham
Done as you said.

@timgraham timgraham commented on an outdated diff Mar 12, 2014
django/forms/forms.py
@@ -618,6 +618,12 @@ def label_tag(self, contents=None, attrs=None, label_suffix=None):
id_for_label = widget.id_for_label(id_)
if id_for_label:
attrs = dict(attrs or {}, **{'for': id_for_label})
+ if (self.field.required and hasattr(self.form, 'required_css_class')):
@timgraham
Django member
timgraham added a note Mar 12, 2014

don't need outer parentheses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Mar 12, 2014
django/forms/forms.py
@@ -618,6 +618,12 @@ def label_tag(self, contents=None, attrs=None, label_suffix=None):
id_for_label = widget.id_for_label(id_)
if id_for_label:
attrs = dict(attrs or {}, **{'for': id_for_label})
+ if (self.field.required and hasattr(self.form, 'required_css_class')):
+ attrs = attrs or {}
+ try:
@timgraham
Django member
timgraham added a note Mar 12, 2014

replace try/except with if 'class' in attrs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Mar 12, 2014
django/forms/forms.py
@@ -618,6 +618,12 @@ def label_tag(self, contents=None, attrs=None, label_suffix=None):
id_for_label = widget.id_for_label(id_)
if id_for_label:
attrs = dict(attrs or {}, **{'for': id_for_label})
+ if (self.field.required and hasattr(self.form, 'required_css_class')):
+ attrs = attrs or {}
+ try:
+ attrs['class'] = attrs['class'] + ' ' + self.form.required_css_class
@timgraham
Django member
timgraham added a note Mar 12, 2014

attrs['class'] += ' ' + self.form.required_css_class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Mar 12, 2014
tests/forms_tests/tests/test_regressions.py
@@ -153,3 +153,15 @@ class Meta:
obj = form.save()
obj.name = 'Camembert'
obj.full_clean()
+
+ def test_regression_17922(self):
@timgraham
Django member
timgraham added a note Mar 12, 2014

It's a bit confusing to put this test here since this isn't a regression but a new feature. The distinction for regression tests is probably a bit arbitrary, but consider trying to find a better place for it. I would name the method some more descriptive like test_label_has_required_css_class and then include a docstring with a description of the behavior and the ticket number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coder9042

@timgraham
Docs added

@timgraham
Django member

Made some minor edits and merged in 416a858.

@timgraham timgraham closed this Mar 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.