Skip to content
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

Fixed #12437 -- Added css_classes to Form._html_output(). #4819

Closed
wants to merge 1 commit into from
Closed

Fixed #12437 -- Added css_classes to Form._html_output(). #4819

wants to merge 1 commit into from

Conversation

alimony
Copy link
Contributor

@alimony alimony commented Jun 5, 2015

@@ -249,7 +250,8 @@ def _html_output(self, normal_row, error_row, row_ender, help_text_html, errors_
'label': '',
'field': '',
'help_text': '',
'html_class_attr': html_class_attr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you make the html_class_attr field empty here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw no real reason for it when creating an empty row for the sole purpose to insert other output. However, I digged through the tickets I could find on this (3512, 3686, 3515), and looked at the original commit adding it, and it seems like it was written like that on purpose, although I find no reference to why. So I just pushed a new commit that reverts it, doesn't make a difference for this new patch :)

@timgraham timgraham changed the title Expose css_classes in _html_output. Fixed #12437 -- Exposed css_classes in _html_output. Jun 6, 2015
@timgraham timgraham changed the title Fixed #12437 -- Exposed css_classes in _html_output. Fixed #12437 -- Added css_classes to Form._html_output(). Jun 6, 2015

def test_field_with_css_class(self):
class SomeForm(Form):
some_field = CharField(widget=TextInput(attrs={'class': 'first_name_id'}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is widget=... important for the test?

@alimony
Copy link
Contributor Author

alimony commented Jun 6, 2015

I think the widget was just a leftover from my dev process, I've removed it now and squashed some of the commits. Should I ideally squash everything into one commit?

@timgraham
Copy link
Member

Yes, please. You can also use the commit message from the title of the pull request (see the patch review checklist for the commit message guidelines).

@alimony
Copy link
Contributor Author

alimony commented Jun 6, 2015

I've squashed everything into one commit with a better message, will pay more attention to this in the future.

@timgraham
Copy link
Member

merged in 1884bf8, thanks!

@timgraham timgraham closed this Jun 6, 2015
@alimony alimony deleted the css_classes_form_html branch June 16, 2015 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants