Skip to content

Dismiss the keyboard when the picker opens on android #5

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

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

bernhardoj
Copy link

@bernhardoj bernhardoj commented Mar 10, 2023

Dismiss the soft keyboard when the picker opens on Android. This will align the behavior with the iOS platform.

Related Issue: Expensify/App#15109

Upstream PR: lawnstarter#501

Copy link
Member

@thesahindia thesahindia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and tests well!

cc: @tgolen

@@ -532,6 +532,12 @@ export default class RNPickerSelect extends PureComponent {
onValueChange={this.onValueChange}
selectedValue={selectedItem.value}
{...pickerProps}
onFocus={() => {
Keyboard.dismiss();
if (pickerProps.onFocus) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about how this repo does it, but in E/App we would also check to make sure that onFocus is also a function before calling it. Should we do that here? It seems like it would be nice, so that the lib doesn't crash if someone passes something that isn't a function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was what I initially thought too. But I check for the other callback (onOpen, onClose) and none of them check if it's a function. I think it's fine to let the app that implement the library to make sure a function is passed, just like React Native component that will throw an error if you give onPress an integer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgolen, thoughts?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think that works for me. I'll be interested to see if they request something similar in the upstream repo's PR.

@tgolen tgolen merged commit 84ee97d into Expensify:master Mar 16, 2023
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.

3 participants