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

feat: focus events management on Android picker #258

Merged

Conversation

mateusz1913
Copy link
Contributor

Hi, I want to introduce two features for Android picker in that PR

  1. Focus events onBlur and onFocus
  2. Possibility of programmatic opening/closing of picker

I updated README and example app with example usage of these new features.

Closes #54 #204 #205 #256
and I guess that it should also be ok for #170

@Lucas-Alvino
Copy link

This PR would be very useful

@Lucas-Alvino
Copy link

@Naturalclar merge plis

@Lucas-Alvino
Copy link

@Naturalclar merge plis

js/Picker.js Outdated
@@ -133,6 +133,7 @@ type PickerProps = $ReadOnly<{|
* </Picker>
*/
class Picker extends React.Component<PickerProps> {
pickerRef = React.createRef();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pickerRef = React.createRef();
pickerRef: React.ElementRef<any> = React.createRef();

js/Picker.js Outdated
Comment on lines 153 to 155
blur = () => {
this.pickerRef.current?.blur();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
blur = () => {
this.pickerRef.current?.blur();
};
blur: () => void = () => {
this.pickerRef.current?.blur();
};

js/Picker.js Outdated
this.pickerRef.current?.blur();
};

focus = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
focus = () => {
focus: () => void = () => {

Copy link
Contributor

@Naturalclar Naturalclar left a comment

Choose a reason for hiding this comment

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

@mateusz1913 thanks for the addition!
there are some FlowType errors that is making the CI fail.
Could you check the places I've commented?

@mateusz1913
Copy link
Contributor Author

Done

Copy link
Contributor

@Naturalclar Naturalclar left a comment

Choose a reason for hiding this comment

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

LGTM! thank you so much! 🎉

@Naturalclar Naturalclar changed the title feat(#54): focus events management on Android picker feat: focus events management on Android picker May 11, 2021
@Naturalclar Naturalclar merged commit 58ad784 into react-native-picker:master May 11, 2021
github-actions bot pushed a commit that referenced this pull request May 11, 2021
# [1.16.0](v1.15.0...v1.16.0) (2021-05-11)

### Features

* focus events management on Android picker ([#258](#258)) ([58ad784](58ad784))
@Naturalclar
Copy link
Contributor

🎉 This PR is included in version 1.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@trandinhduc90
Copy link

Hi @mateusz1913 I got a problem with CI build because the ReactPickerManager.java file has two functions receiveCommand with two @Override Android will build failed

@mateusz1913
Copy link
Contributor Author

Hi @trandinhduc90, without any kind of logs or some error feedback I am not able to help

@trandinhduc90
Copy link

Hi @trandinhduc90, without any kind of logs or some error feedback I am not able to help

Task :@react-native-picker_picker:compileReleaseJavaWithJavac FAILED /Users/runner/work/1/s/node_modules/@react-native-picker/picker/android/src/main/java/com/reactnativecommunity/picker/ReactPickerManager.java:161: error: method does not override or implement a method from a supertype @Override

This is error when I run CI build

@mateusz1913
Copy link
Contributor Author

mateusz1913 commented Jun 13, 2021

So it seems that you have some RN old version. Take a look here (these methods in base class): ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java in react-native repo (lines 201 and 212)

Method with int argument is deprecated (at least for newer RN versions). From your error it seems that there is sth wrong with new receiveCommand override (that one with String argument)

What RN version do you use?

@trandinhduc90
Copy link

So it seems that you have some RN old version. Take a look here (these methods in base class): ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java in react-native repo (lines 201 and 212)

Method with int argument is deprecated (at least for newer RN versions). From your error it seems that there is sth wrong with new receiveCommand override (that one with String argument)

What RN version do you use?

I guess that about my react-native version. Currently, I'm using 0.60.5 so maybe I will try to upgrade my react-native version to 0.63 and gave you feedback soon. So thanks for the reply.

@mateusz1913
Copy link
Contributor Author

mateusz1913 commented Jun 13, 2021

facebook/react-native@3cae6fa

facebook/react-native@6eec393

These are commits that included second receiveCommand override and deprecated first one

They are included on 0.61+ branches, so you definitely need to upgrade to at least 0.61

@Naturalclar maybe percentage of devs using old RN versions is low, but it would be good to add small note in README, that focus management is available from 0.61+, if I'll have time I can make it, but maybe someone will be quicker

@trandinhduc90
Copy link

facebook/react-native@3cae6fa

facebook/react-native@6eec393

These are commits that included second receiveCommand override and deprecated first one

They are included on 0.61+ branches, so you definitely need to upgrade to at least 0.61

@Naturalclar maybe percentage of devs using old RN versions is low, but it would be good to add small note in README, that focus management is available from 0.61+, if I'll have time I can make it, but maybe someone will be quicker

Thanks @mateusz1913 , I will try to upgrade to 0.61. So some company still using old react-native version because there don't has the resource to maintain there app.

@omaryoussef
Copy link

I don't know if anyone is still looking at this PR, but using a ref to execute the focus method comes back with an error pickerRef$current.focus is not a function. Any workarounds? It seems like it's not just me: #276

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

Successfully merging this pull request may close these issues.

Picker component does not have an onTouch property
6 participants