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

[Hold for Payment August 4th] IOU - Autofocus 'What's it for?' text box in details page #4215

Closed
2 of 5 tasks
isagoico opened this issue Jul 24, 2021 · 18 comments
Closed
2 of 5 tasks
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Jul 24, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Request money to a user
  2. Enter an amount and tap on request

Expected Result:

Keyboard should come up and auto focused to the "What's it for?"

Actual Result:

Keyboard is not focused to the "What's it for box?" box and user has to manually tap on it to write.

Workaround:

User has to manually tap the text box.

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.0.80-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

Upwork Issue URL: https://www.upwork.com/jobs/~0100cc108c7250028f

View all open jobs on Upwork


From @joekaufmanexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1626898032104700

The details page for an IOU request should autofocus on the What's it for? field and bring up the keyboard automatically. Also, the green button should float and sit above the keyboard when you tap Optional and open the keyboard. Both of these are happening on iOS. Screenshots in

@MelvinBot
Copy link

Triggered auto assignment to @bondydaa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@rushatgabhane
Copy link
Member

If you make it External

Proposal

In components/IOUConfirmationList

add autoFocus prop to the TextInput

@aman-atg
Copy link
Contributor

Proposal

  • Inside

    <TextInput
    style={[styles.textInput]}
    value={this.props.comment}
    onChangeText={this.props.onUpdateComment}
    placeholder={this.props.translate('common.optional')}
    placeholderTextColor={themeColors.placeholderText}
    />

  • canFocusInputOnScreenFocus should be used here with autoFocus

  • It'll look like this :
    image

@rushatgabhane
Copy link
Member

rushatgabhane commented Jul 24, 2021

@aman-atg

canFocusInputOnScreenFocus should be used here with autoFocus

Hmm, why? It just returns false, so it won't work.

@aman-atg
Copy link
Contributor

@aman-atg

canFocusInputOnScreenFocus should be used here with autoFocus

Hmm, why? It just returns false, so it won't work.

@bondydaa
Copy link
Contributor

Feel pretty confident since we've already got 2 contributors commenting that we can make this External 😁 .

I do see that we've got some usages of canUseTouchScreen but it's not used everywhere right now:

Ex (note I know some of these are default props/initial state - just sharing for context):

Expensidev/App (main) $ ack -C 5 autoFocus
src/components/AddPlaidBankAccount.js
183-                                        style={[styles.textInput, styles.mb2]}
184-                                        value={this.state.password}
185-                                        autoCompleteType="password"
186-                                        textContentType="password"
187-                                        autoCapitalize="none"
188:                                        autoFocus={canFocusInputOnScreenFocus()}
189-                                        onChangeText={text => this.setState({password: text})}
190-                                    />
191-                                </View>
192-                            )}
193-                        </View>

src/components/TextInputFocusable/index.android.js
21-
22-    /** When the input has cleared whoever owns this input should know about it */
23-    onClear: PropTypes.func,
24-
25-    /** Set focus to this component the first time it renders.
26:     * Override this in case you need to set focus on one field out of many, or when you want to disable autoFocus */
27:    autoFocus: PropTypes.bool,
28-
29-    /** Prevent edits and interactions like focus for this input. */
30-    isDisabled: PropTypes.bool,
31-
32-};
33-
34-const defaultProps = {
35-    shouldClear: false,
36-    onClear: () => {},
37:    autoFocus: false,
38-    isDisabled: false,
39-    forwardedRef: null,
40-};
41-
42-class TextInputFocusable extends React.Component {

src/components/TextInputFocusable/index.js
48-
49-    /** Whether or not this TextInput is disabled. */
50-    isDisabled: PropTypes.bool,
51-
52-    /** Set focus to this component the first time it renders.
53:    Override this in case you need to set focus on one field out of many, or when you want to disable autoFocus */
54:    autoFocus: PropTypes.bool,
55-
56-    /** Update selection position on change */
57-    onSelectionChange: PropTypes.func,
58-
59-    /** Selection Object */
--
76-    onDragEnter: () => {},
77-    onDragOver: () => {},
78-    onDragLeave: () => {},
79-    onDrop: () => {},
80-    isDisabled: false,
81:    autoFocus: false,
82-    forwardedRef: null,
83-    onSelectionChange: () => {},
84-    selection: {
85-        start: 0,
86-        end: 0,

src/components/TextInputFocusable/index.ios.js
21-
22-    /** When the input has cleared whoever owns this input should know about it */
23-    onClear: PropTypes.func,
24-
25-    /** Set focus to this component the first time it renders.
26:     * Override this in case you need to set focus on one field out of many, or when you want to disable autoFocus */
27:    autoFocus: PropTypes.bool,
28-
29-    /** Prevent edits and interactions like focus for this input. */
30-    isDisabled: PropTypes.bool,
31-
32-    /** Selection Object */
--
38-};
39-
40-const defaultProps = {
41-    shouldClear: false,
42-    onClear: () => {},
43:    autoFocus: false,
44-    isDisabled: false,
45-    forwardedRef: null,
46-    selection: {
47-        start: 0,
48-        end: 0,

src/pages/home/report/ReportActionItemMessageEdit.js
128-                        style={[styles.textInputCompose, styles.flex4]}
129-                        onFocus={() => {
130-                            scrollToIndex({animated: true, index: this.props.index}, true);
131-                            toggleReportActionComposeView(false);
132-                        }}
133:                        autoFocus
134-                        selection={this.state.selection}
135-                        onSelectionChange={this.onSelectionChange}
136-                    />
137-                </View>
138-                <View style={[styles.flexRow, styles.mt1]}>

src/pages/home/report/ReportActionCompose.js
534-                                            />
535-                                        </>
536-                                    )}
537-                                </AttachmentPicker>
538-                                <TextInputFocusable
539:                                    autoFocus={this.shouldFocusInputOnScreenFocus}
540-                                    multiline
541-                                    ref={this.setTextInputRef}
542-                                    textAlignVertical="top"
543-                                    placeholder={inputPlaceholder}
544-                                    placeholderTextColor={themeColors.placeholderText}

src/pages/home/report/EmojiPickerMenu/index.js
343-                            placeholderTextColor={themeColors.textSupporting}
344-                            onChangeText={this.filterEmojis}
345-                            style={styles.textInput}
346-                            defaultValue=""
347-                            ref={el => this.searchInput = el}
348:                            autoFocus
349-                            selectTextOnFocus={this.state.selectTextOnFocus}
350-                        />
351-                    </View>
352-                )}
353-                {this.state.filteredEmojis.length === 0

src/pages/signin/PasswordForm.js
90-                        autoCompleteType="password"
91-                        textContentType="password"
92-                        value={this.state.password}
93-                        onChangeText={text => this.setState({password: text})}
94-                        onSubmitEditing={this.validateAndSubmitForm}
95:                        autoFocus
96-                    />
97-                </View>
98-
99-                {this.props.account.requiresTwoFactorAuth && (
100-                    <View style={[styles.mv3]}>

src/pages/signin/LoginForm.js
83-                        autoCapitalize="none"
84-                        autoCorrect={false}
85-                        keyboardType={getEmailKeyboardType()}
86-                        placeholder={this.props.translate('loginForm.phoneOrEmail')}
87-                        placeholderTextColor={themeColors.placeholderText}
88:                        autoFocus={canFocusInputOnScreenFocus()}
89-                    />
90-                </View>
91-                <View style={[styles.mt5]}>
92-                    <Button
93-                        success

If this is the right method to use (not 100% sure here since I'm not super familiar with new expensify code) then we should probably make sure it's used everywhere properly so that as more people contribute and see instances of how to do autoFocus it's all being handled in the same way.

cc @AndrewGable @roryabraham @marcaaron @tgolen to gut check me here 🙇

@MelvinBot MelvinBot removed the Overdue label Jul 26, 2021
@bondydaa bondydaa added the External Added to denote the issue can be worked on by a contributor label Jul 26, 2021
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@bondydaa bondydaa removed their assignment Jul 26, 2021
@roryabraham
Copy link
Contributor

roryabraham commented Jul 26, 2021

Hmmm I can't think of any reason that we would want to auto-focus this form input only on touch devices ... so I'm not sure canUseTouchScreen is relevant.

TL;DR I think that @rushatgabhane's proposal looks good 👍


One thing to be aware of that we've run into in the past...There can be bugs with the react-navigation screen transition animation related to autoFocus. In the past, I had to fix one such bug with this PR, which removed the use of the autoFocus prop.

If (and only if) that ends up being something we need to do here, we should also be aware of some iOS safari quirks. Basically, iOS Safari really doesn't like websites using programatically triggered events to open the software keyboard. However, paradoxically, it does natively support the autoFocus attribute for text inputs ... really helpful and consistent Apple 🙄 ... Anyways, because that is a purposeful design choice enforced by Apple, let's not worry about implementing a workaround for iOS Safari in the event we can't use autoFocus.

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 26, 2021
@MelvinBot
Copy link

Triggered auto assignment to @roryabraham (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@trjExpensify
Copy link
Contributor

Created the job to apply here @rushatgabhane 👍

@arpitdeveloper
Copy link

In src/components/IOUConfirmationList.js
we add autoFocus = {true} in textInput

@rushatgabhane
Copy link
Member

Thanks for the heads up. I just got a macbook, will test for it.

One thing to be aware of that we've run into in the past...There can be bugs with the react-navigation screen transition animation related to autoFocus. In the past, I had to fix one such bug with this PR, which removed the use of the autoFocus prop.

If (and only if) that ends up being something we need to do here, we should also be aware of some iOS safari quirks. Basically, iOS Safari really doesn't like websites using programatically triggered events to open the software keyboard. However, paradoxically, it does natively support the autoFocus attribute for text inputs ... really helpful and consistent Apple 🙄 ... Anyways, because that is a purposeful design choice enforced by Apple, let's not worry about implementing a workaround for iOS Safari in the event we can't use autoFocus.

@trjExpensify trjExpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 27, 2021
@trjExpensify
Copy link
Contributor

Accepted the proposal on Upwork and assigned you the issue, @rushatgabhane. 👍

@roryabraham
Copy link
Contributor

PR was merged and deployed to staging yesterday.

@MelvinBot MelvinBot removed the Overdue label Jul 29, 2021
@trjExpensify trjExpensify changed the title IOU - Autofocus 'What's it for?' text box in details page [Hold for Payment August 4th] IOU - Autofocus 'What's it for?' text box in details page Jul 29, 2021
@trjExpensify trjExpensify added Weekly KSv2 and removed Daily KSv2 labels Jul 29, 2021
@MelvinBot
Copy link

@trjExpensify, @rushatgabhane, @roryabraham it looks like no one is assigned to work on this job.
Please double the price or add a comment stating why the job isn't being doubled.

@roryabraham
Copy link
Contributor

@rushatgabhane completed this job and it was deployed to staging 6 days ago

@roryabraham
Copy link
Contributor

@trjExpensify Can we confirm that @rushatgabhane was paid for this issue then close it out?

@MelvinBot MelvinBot removed the Overdue label Aug 12, 2021
@trjExpensify
Copy link
Contributor

Yep, paid and closed out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants