-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Dialog] clear deprecated api #2396
Conversation
Looks good. What should we do with the |
Is there any case for uncontrolled |
I think that uncontrolled DatePicket and TimePicker makes sense. |
@oliviertassinari I'm confused O.o |
What I mean is that controlled and uncontrolled are two valid use case. |
@oliviertassinari I see. In that case, sure. But let's not introduce breaking changes right now to those components. I'll try to patch them up within this PR before merging. sounds good? Later on we'll deprecate those props and cover the use-cases in a more convenient way. |
Sounds awesome! |
@@ -193,12 +183,14 @@ const DatePickerDialog = React.createClass({ | |||
}, | |||
|
|||
show() { | |||
if (this.props.onShow && !this.state.open) this.props.onShow(); |
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.
the !this.state.open
makes sure show (and dismiss) events are only fired once. prevents consecutive dispatch when going berserk with clicks!
e1c9dc3
to
921d06a
Compare
@oliviertassinari Is this ok? I think it's better not to rebase, the second commit indicates exactly why those lines where removed and added. |
Looks fine :). |
[Dialog] clear deprecated api
How about releasing new version to NPM? Currently DatePicker is broken in 0.14.0-rc1 version. |
@oliviertassinari I had to remove isOpen() since it makes no sense now. And defaultOpen, I don't think there really is a use-case for this. therefore I made open required. do you think we should ease into this or YOLO?!