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

Added allowTextInput property #1107

Merged
merged 3 commits into from
Feb 7, 2022
Merged

Conversation

milanholemans
Copy link
Contributor

Q A
Bug fix? [ ]
New feature? [ x ]
New sample? [ ]
Related issues? fixes #1094

What's in this Pull Request?

Exposed the allowTextInput property. Developer can now choose whether the user can add dates as text value rather than picking one from the date picker.

@joelfmrodrigues
Copy link
Collaborator

@milanholemans many thanks for the PR. I just noticed that the previous default value was true and now is false so wondering if this could cause issues to people who upgrade and don't notice the change. Could this be set to true by default, or did you experience any issues?

@milanholemans
Copy link
Contributor Author

@milanholemans many thanks for the PR. I just noticed that the previous default value was true and now is false so wondering if this could cause issues to people who upgrade and don't notice the change. Could this be set to true by default, or did you experience any issues?

The thing is, I noticed that the DatePicker component of office-fabric-ui uses false as default value. That's why I made it false here by default as well. I can change it to true if needed.

@joelfmrodrigues
Copy link
Collaborator

@AJIXuMuK what are your thoughts regarding changing this default value? Should we avoid it or go ahead and ensure that it's documented/announced?

@milanholemans
Copy link
Contributor Author

It has been changed from false to true last year, check this PR #971.

@AJIXuMuK
Copy link
Collaborator

@joelfmrodrigues I would not change the defaults without a necessity.
@milanholemans - thank you for providing this info.
This is our mistake. We should have implemented this as property right away.
But for now - let's leave the default as is.

@milanholemans
Copy link
Contributor Author

milanholemans commented Jan 30, 2022

@AJIXuMuK @joelfmrodrigues I changed the default value back to true.

@joelfmrodrigues joelfmrodrigues merged commit b1602c4 into pnp:dev Feb 7, 2022
@joelfmrodrigues
Copy link
Collaborator

@milanholemans many thanks, it has now been merged

@joelfmrodrigues joelfmrodrigues added the status:fixed-next-drop Issue will be fixed in upcoming release. label Feb 7, 2022
@joelfmrodrigues joelfmrodrigues added this to the 3.6.0 milestone Feb 7, 2022
@joelfmrodrigues joelfmrodrigues removed the status:fixed-next-drop Issue will be fixed in upcoming release. label Feb 7, 2022
@milanholemans milanholemans deleted the allowTextInput branch April 8, 2022 21:29
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