Don't focus input until screen transition has ended #3719
Conversation
|
|
| @@ -72,6 +72,8 @@ class AddSecondaryLoginPage extends Component { | |||
| this.formType = props.route.params.type; | |||
| this.submitForm = this.submitForm.bind(this); | |||
| this.validateForm = this.validateForm.bind(this); | |||
|
|
|||
| this.phoneNumberInputRef = null; | |||
There was a problem hiding this comment.
I don't think it's necessary to add this here and it's fine that it stays undefined.
There was a problem hiding this comment.
Marc and I added a rule to the style guide about this a few months ago: https://github.com/Expensify/Expensify.cash/blob/main/STYLE.md#use-refs-appropriately:
all refs should be declared in the constructor, rather than inline. This makes it easier to quickly see what refs are declared in the component
Is this something you disagree with?
There was a problem hiding this comment.
Yeah, I do disagree with that.
Defining them in the constructor doesn't do anything (computationally). I think we should avoid code that doesn't do anything, just to make something easier to find. If I follow the current style guide suggestion and apply it to other situations, that would lead us to a place where all variables and class properties are defined in the constructor. I just think it makes it messy and bloats the code without any real purpose.
all refs should be declared in the constructor, rather than inline
I also think that this isn't really correct. It's not being "declared" inline, it's just being assigned a value inline, which doesn't change even if the variable is initialized in the constructor.
This makes it easier to quickly see what refs are declared in the component
This implies that the problem being solved was "refs are hard to find and locate", which I think is a problem that I don't agree with. Maybe there is a good example of where refs are hard to find and we can narrow down why they are hard to find?
There was a problem hiding this comment.
Defining them in the constructor doesn't do anything (computationally). I think we should avoid code that doesn't do anything, just to make something easier to find.
It's true this doesn't change anything and declaring refs in the constructor isn't necessary – the sole purpose is to improve readability.
This implies that the problem being solved was "refs are hard to find and locate", which I think is a problem that I don't agree with.
Yeah, that was the implied problem, but I do think it's pretty subjective. Since it's not unanimous that this rule improves the readability of components that use refs, I think it makes sense to err on the side of not adding lines of code that don't do anything.
This style rule was introduced in this PR, so I'm going to migrate this discussion over there. If others agree with your reasoning here, then we should be able to just revert that PR.
There was a problem hiding this comment.
Cool, thanks for moving that convo over to that PR and for listening!
|
@parasharrajat and I found a workaround for the keyboard not opening on focus in iOS Safari, but it comes with a consequence where the weird half-transition bug displays for a moment before the keyboard will open. We think it's better not to implement that workaround, because it negatively affects all platforms in order to provide a workaround for Safari. This PR in its current state works on all platforms but requires one extra click on iOS safari, which is not a big deal |
|
Updated |
Don't focus input until screen transition has ended (cherry picked from commit c94c24a)
|
🚀 Cherry-picked to staging in version: 1.0.73-1🚀
|
|
🚀 Deployed to production in version: 1.0.73-3🚀
|
|
🚀 Deployed to staging in version: 1.0.73-4🚀
|
|
🚀 Deployed to production in version: 1.0.74-0🚀
|
Details
Don't focus the input until the screen transition animation has completed.
Fixed Issues
Fixes #3604
Tests / QA Steps
Tested on
Screenshots
Web
Mobile Web
Desktop
iOS
Android