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

Close overlay with closeOnSelect if open prop is set #741

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Close overlay with closeOnSelect if open prop is set #741

wants to merge 1 commit into from

Conversation

danrot
Copy link

@danrot danrot commented Oct 5, 2020

Description

I have adjusted the logic happening when the onCloseSelect prop is set. It now also calls the onClose callback if the open prop is set to some value.

Motivation and Context

In v2 of this library we were using this component somehow like this (simplified example):

<ReactDateTime
    closeOnSelect={true}
    onClose={this.handleClose}
    open={this.props.open}
/>

This doesn't seem to work anymore in v3, because the closeOnSelect prop does not have any effect if any value other than undefined is passed to the open prop.

If this PR is accepted, the handleClose method will be called as expected. It is still in control of the rendering component if it changes the value passed to the open prop, in case that was a concern.

Checklist

[ ] I have not included any built dist files (us maintainers do that prior to a new release)
[x] I have added tests covering my changes
[x] All new and existing tests pass
[ ] My changes required the documentation to be updated
  [ ] I have updated the documentation accordingly
  [ ] I have updated the TypeScript 1.8 type definitions accordingly
  [ ] I have updated the TypeScript 2.0+ type definitions accordingly

@danrot
Copy link
Author

danrot commented Oct 5, 2020

The checklist says I should not include any built dist files, but if I don't the tests and therefore the precommit hook fail 😕

@danrot
Copy link
Author

danrot commented Oct 13, 2020

@arqex Any chance that this is merged? This is a breaking change, that would force me to rewrite quite a bit of my application logic.

@onlined
Copy link
Collaborator

onlined commented Oct 15, 2020

@danrot You can use onChange prop for your purpose, like this:

<ReactDateTime
    onChange={this.handleClose}
	{...otherProps}
/>

@danrot
Copy link
Author

danrot commented Oct 16, 2020

@onlined I know that I could rewrite it this way, that's what I meant in my previous comment 🙂 I would just like to avoid it, and I still think that the behavior "fixed" in this PR is very confusing, and it was working differently in a previous version. In addition to that I cannot see how anybody would benefit from that change, so I guess it was on accident.

@alexander-schranz
Copy link
Contributor

@arqex any update to this?

@alexander-schranz
Copy link
Contributor

@onlined the problem with the onChange is that it also close automatically then when the time is changed and thats a behaviour you don't want e.g.:

datetime-picker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants