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

[Tech debt] Rich-text use new functions blurField and focusField in onBlur and onFocus #1711

Open
dratwas opened this issue Dec 20, 2019 · 5 comments

Comments

@dratwas
Copy link
Contributor

dratwas commented Dec 20, 2019

I think there is no reason to call focus in _onFocus callback and blur in _onBlur callback in AztecView.
We should only set the correct ID since we already know that the input is focused/blurred.

At this moment there is no such a functionality but blurField and focusField will be introduced in react-native 0.62.

See #1696
and #1702 (comment)

@chipsnyder
Copy link
Contributor

chipsnyder commented Feb 10, 2021

I'm going through and reviewing some of the open issues on our boards to make sure I understand the current state.

@dratwas Now that we've updated to 0.62 what would be the ideal change needed to resolve this task? It seems like it would be to switch something to blurField or focusField but I'm not sure I understand what or why without digging deeper.

@dratwas
Copy link
Contributor Author

dratwas commented Feb 11, 2021

@chipsnyder

Now that we've updated to 0.62 what would be the ideal change needed to resolve this task?

Unfortunately, we are still on 0.61.5 ;( https://github.com/WordPress/gutenberg/blob/master/package.json#L193

It seems like it would be to switch something to blurField or focusField but I'm not sure I understand what or why without digging deeper.

I believe that this comment should help :) #1702 (comment)
Unfortunately, some of the links are outdated because of the migration to monorepo, however, the main thought remains the same.

@chipsnyder
Copy link
Contributor

chipsnyder commented Feb 11, 2021

Unfortunately, we are still on 0.61.5

😅 oops I must have gotten confused by looking at the bump to 0.63 work never mind

I believe that this comment should help :) #1702 (comment)

I actually got a bit lost in that comment's description and what the goal would be once we update to 0.63.

If the desired state is to delete those two lines can we go ahead and do that?
-Or-
If the desired state is to switch to using blurField or focusField?

@dratwas
Copy link
Contributor Author

dratwas commented Feb 11, 2021

If the desired state is to delete those two lines can we go ahead and do that?
-Or-
If the desired state is to switch to using blurField or focusField?

The desired state is

AztecView function 0.62 0.63
_onFocus focusField(textFieldID: ?number) focusInput(textField: ?ComponentRef)
_onBlur blurField(textFieldID: ?number) blurInput(textField: ?ComponentRef)
focus focusTextInput(textFieldID: ?number) focusTextInput(textField: ?ComponentRef)
blur blurTextInput(textFieldID: ?number) blurTextInput(textField: ?ComponentRef)

I can see that it is already done on the update-rn-0-63 branch but with one difference. The blurTextInput is used instead of blurInput in the _onBlur. I suggest using blurInput because if the _onBlur is called that means the field is already blurred and we don't need to blur it again. It's enough to clear currentlyFocusedInputRef on the JS side w/o sending events to the native.

In addition, I also think that the whole logic should be revisited after these changes since it's kinda confusing that we call
focus() inside the _onFocus() on iOS. We should only change the currentlyFocusedInputRef on the JS side using focusInput w/o sending events to native.

@chipsnyder
Copy link
Contributor

I think I understand now. Thank you @dratwas for summarizing it again!

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

No branches or pull requests

2 participants