Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Close overlay with closeOnSelect if open prop is set#741

Open
danrot wants to merge 1 commit into
arqex:master from
danrot:close-on-select-with-open-prop
Open

Close overlay with closeOnSelect if open prop is set #741
danrot wants to merge 1 commit into
arqex:master from
danrot:close-on-select-with-open-prop

Conversation

@danrot

@danrot danrot commented Oct 5, 2020 •
edited
Loading

Copy link
Copy Markdown

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

alexander-schranz and niklasnatter reacted with thumbs up emoji

danrot commented Oct 5, 2020

Copy link
Copy Markdown
Author

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 commented Oct 13, 2020

Copy link
Copy Markdown
Author

@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 commented Oct 15, 2020

Copy link
Copy Markdown
Collaborator

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

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

danrot commented Oct 16, 2020

Copy link
Copy Markdown
Author

@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.

Copy link
Copy Markdown
Contributor

@arqex any update to this?

Copy link
Copy Markdown
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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /