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 #27723 -- Passed MultiWidget's attrs argument to widgets constructors to set the input type. #7835

Merged
merged 1 commit into from Jan 13, 2017

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Jan 11, 2017

Ticket 27723.
Regression in b52c730.

@timgraham
Copy link
Member

This is confusing me because you say it's a regression but the tests (at least not all of them) are passing on 1.10.

@felixxm
Copy link
Member Author

felixxm commented Jan 11, 2017

Now I'm also confused:), maybe I shouldn't used word "regression". In Django 1.10 MultiWidget passed type to all its widgets contexts properly, now it's duplicated.

@timgraham
Copy link
Member

I understand now. I left a comment on the ticket about a possible alternate approach as I'm not sure the new behavior of applying attrs to all subwidgets is desirable.

@felixxm
Copy link
Member Author

felixxm commented Jan 12, 2017

It's a behavior that some of Django 3rd party apps expecting, e.g. django-filters (see test_widgets.py#L139). I think that approach from ticket 5851 is a issue specific for SplitDateTimeWidget, because in general you can get the same in a different way

MultiWidget(
    widgets=(
        Input(attrs={'class': 'big', 'type': 'text'}),
        Input(attrs={'class': 'small', 'type': 'date'}),
    )
)

Now we apply attrs to all subwidgets contexts, but without passing them to Widget.__init__ we can miss some specific behavior like with type in Input.

Copy link
Member

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

Is the behavior of attrs documented for MultiWidget? I'm concerned this changes more behavior than just fixing the regression. Maybe those are other bug fixes but it's difficult to review and understand exactly what is changing and why.

'<input type="number" value="2" name="code_1" />'
'<input type="text" value="3" name="code_2" />'
))
widget = MyMultiWidget(widgets=(TextInput(attrs), TextInput(attrs)), attrs={'attrs': 'date'})
Copy link
Member

Choose a reason for hiding this comment

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

What happened to {'attrs': 'date'}? On 1.10, the output is '<input attrs="date" name="code_0" type="number" value="1" /><input attrs="date" name="code_1" type="text" value="2" />'.

TextInput(),
), attrs=attrs)
self.check_html(widget, 'code', ['1', '2', '3'], html=(
'<input type="number" value="1" name="code_0" />'
Copy link
Member

Choose a reason for hiding this comment

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

On 1.10, the output is: '<input type="number" value="1" name="code_0" />' '<input type="number" value="2" name="code_1" />' '<input type="number" value="3" name="code_2" />'.

@felixxm
Copy link
Member Author

felixxm commented Jan 12, 2017

@timgraham Thanks for review. In the second commit I tried to restore correct behavior from 1.10 without passing attrs to subwidgets constructors.

@@ -17,6 +17,11 @@ def decompress(self, value):
return ['', '']


class MyMultiRenderWidget(MultiWidget):
Copy link
Member

Choose a reason for hiding this comment

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

Is this test applicable to the patch anymore? It doesn't seem to me that overriding attrs with render() is related to what this patch fixes.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I removed it.

@@ -118,6 +123,42 @@ def test_constructor_attrs(self):
'<input id="bar_1" type="text" class="small" value="lennon" name="name_1" />'
))

def test_constructor_attrs_with_type(self):
attrs = {'type': 'number'}
widget = MyMultiWidget(widgets=(
Copy link
Member

Choose a reason for hiding this comment

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

Single line looks okay here. Are 3 widgets needed rather than two?

Copy link
Member Author

@felixxm felixxm Jan 13, 2017

Choose a reason for hiding this comment

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

I used 3 widgets to check that attrs is not changed inside their constructors, but now it's unnecessary.

@timgraham timgraham merged commit 974d145 into django:master Jan 13, 2017
@felixxm felixxm deleted the issue-27723 branch January 13, 2017 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants