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

[DatePicker] first argument null for datepicker onchange #441

Closed
roodboi opened this issue Mar 18, 2015 · 3 comments
Closed

[DatePicker] first argument null for datepicker onchange #441

roodboi opened this issue Mar 18, 2015 · 3 comments
Labels
bug 🐛 Something doesn't work

Comments

@roodboi
Copy link

roodboi commented Mar 18, 2015

is there a reason why the date isnt returned as the first argument? https://github.com/callemall/material-ui/blob/master/src/js/date-picker/date-picker.jsx#L93

@jkruder
Copy link
Contributor

jkruder commented Mar 18, 2015

Because onChange is also passed to the TextField https://github.com/callemall/material-ui/blob/master/src/js/date-picker/date-picker.jsx#L64 and it has an event as its first argument. Looks like the intent is to funnel changes made in the textfield or dialog. Wouldn't hurt to be specific, property expansion can be confusing.

I suggest renaming DatePicker.onChange to DatePicker.onDateChange and then wire up onDateChange to TextField.onChange and DatePickerDialog.onAccept.

@hai-cea
Copy link
Member

hai-cea commented Jun 18, 2015

@jkruder Sounds like a reasonable approach.

@hai-cea hai-cea changed the title first argument null for datepicker onchange [DatePicker] first argument null for datepicker onchange Jun 18, 2015
@alitaheri alitaheri added the bug 🐛 Something doesn't work label Dec 8, 2015
EugeneZ added a commit to EugeneZ/material-ui that referenced this issue Feb 4, 2016
DatePicker onChange should be onChangeDate
EugeneZ added a commit to EugeneZ/material-ui that referenced this issue Feb 4, 2016
@oliviertassinari
Copy link
Member

@roodboi Yes, there is a good reason. That's for making our onChange event consistent across Material-UI (#2957).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

No branches or pull requests

5 participants