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.

Sign up for GitHub

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

Don't focus input until screen transition has ended #3719

Merged
merged 3 commits into from
Jun 22, 2021
Merged

Don't focus input until screen transition has ended #3719

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
990d665
Don't focus input until screen transition has ended
roryabraham Jun 22, 2021
82ff7ff
Use onTransitionEnd instead of duplicating state
roryabraham Jun 22, 2021
9df65ad
Fix variable naming and add falsey check for function
roryabraham Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/pages/settings/AddSecondaryLoginPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to add this here and it's fine that it stays undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks for moving that convo over to that PR and for listening!

}

componentWillUnmount() {
Expand Down Expand Up @@ -102,7 +104,12 @@ class AddSecondaryLoginPage extends Component {

render() {
return (
<ScreenWrapper>
<ScreenWrapper onTransitionEnd={() => {
if (this.phoneNumberInputRef) {
this.phoneNumberInputRef.focus();
}
}}
>
<KeyboardAvoidingView>
<HeaderWithCloseButton
title={this.props.translate(this.formType === CONST.LOGIN_TYPE.PHONE
Expand All @@ -125,10 +132,10 @@ class AddSecondaryLoginPage extends Component {
: 'profilePage.emailAddress')}
</Text>
<TextInput
ref={el => this.phoneNumberInputRef = el}
style={styles.textInput}
value={this.state.login}
onChangeText={login => this.setState({login})}
autoFocus
keyboardType={this.formType === CONST.LOGIN_TYPE.PHONE
? CONST.KEYBOARD_TYPE.PHONE_PAD : undefined}
returnKeyType="done"
Expand Down