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 2022-01-11] Mobile apps - User is unable to paste an amount in the IOU amount modal #6285

Closed
isagoico opened this issue Nov 12, 2021 · 41 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2

Comments

@isagoico
Copy link

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. Copy any amount to the clipboard (e.g 1232.32)
  2. Navigate to a conversation in NewDot
  3. Request money
  4. Try to paste the amount you copied before in the mount field

Expected Result:

User is able to paste content in the amount field in mobile

Actual Result:

User is unable to paste content in the amount field in mobile

Workaround:

Enter the amount manually

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.1.14-0

Reproducible in staging?: Yes
Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @quinthar
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1636688055149900

View all open jobs on GitHub

@MelvinBot
Copy link

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

@isagoico isagoico changed the title User is unable to paste an amount in the IOU amount modal Mobile apps - User is unable to paste an amount in the IOU amount modal Nov 12, 2021
@tgolen tgolen added Weekly KSv2 External Added to denote the issue can be worked on by a contributor and removed Daily KSv2 labels Nov 12, 2021
@MelvinBot
Copy link

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

@tgolen tgolen added the Improvement Item broken or needs improvement. label Nov 12, 2021
@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Nov 12, 2021
@tgolen tgolen added Weekly KSv2 and removed Daily KSv2 labels Nov 12, 2021
@akshayasalvi
Copy link
Contributor

Proposal

This is happening because we're using a Text component and not TextInput for mobile devices.

My solution is to remove the isSmallScreenWidth check and keep only one block:

<TextInputAutoWidth
        inputStyle={styles.iouAmountTextInput}
        textStyle={styles.iouAmountText}
        value={this.state.amount}
        onChangeText={this.updateAmount}
        ref={el => this.textInput = el}
        placeholder="0"
        showSoftInputOnFocus={false}
    />

I've added showSoftInputOnFocus which ensures that the keyboard doesn't show up on input focus in the mobile apps and our number pad is being able to be used.

image

@rushatgabhane
Copy link
Member

rushatgabhane commented Nov 14, 2021

@akshayasalvi Ideally showSoftInputOnFocus=false would work.

But it has a bug. When you re-open the app from background, the system keyboard shows up!

screen-20211114-073353.mp4

@akshayasalvi
Copy link
Contributor

Thanks for reporting @rushatgabhane.

I’ll probably also add this: (yet to test on a real device):


<TouchableWithoutFeedback onPress={Keyboard.dismiss} > <TextInput /> </TouchableWithoutFeedback>

@rushatgabhane
Copy link
Member

Nope, that didn't help.

@akshayasalvi
Copy link
Contributor

Okay, thanks for notifying me. I'll check something that works on the background too.

@Christinadobrzyn
Copy link
Contributor

I don't see a duplicate job in Expensify/App so created job in Upwork:

Internal job posting - https://www.upwork.com/ab/applicants/1460125754131181568/job-details
External job posting - https://www.upwork.com/jobs/~0192adfe14df539b43

@botify botify removed the Weekly KSv2 label Nov 15, 2021
@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 15, 2021
@elemanhillary
Copy link

Initially you can also use a text component to grab things off the clipboard. I have a simple solution

Solution 1

  • Use a text input

Solution 2

  • Grab text from clipboard

@Amarpsp10
Copy link

Proposal

I have one solution for this without using the TextInput component
we can access the Clipboard value by using @react-native-community/clipboard

 import Clipboard from '../../../libs/Clipboard/index.native'
 .....
 async componentDidMount() {
    .....
    const copiedText = await Clipboard.getString();
    this.setState({copied : copiedText});
}

I tested this above code it's perfectly working !
we will update state every time when user switches App or pages,
after getting the Copied Text, Amount Page will render a suggestion to paste the copied text,

here I created a example image to show how it is going to look like
Frame 2
ke

when user click on this suggested text it will update the Text component.
we need to handle some other things also like

  • showing suggestion only one time
  • Checking for new copied text by comparing already exist one in state

@tgolen
Copy link
Contributor

tgolen commented Nov 16, 2021

Thanks for the proposals!

@akshayasalvi I think ideally, I like your proposal the most. It seems the most simple and the most "best practice" way. Is it possible to find a solution to that background bug?

@elemanhillary It sounds like you are proposing similar things as the others, but I would need to see a more detailed explanation of what solution you would recommend as well as how it would be accomplished.

@Amarpsp10 It looks like you've explored the solution for accessing contents from the clipboard well, however, I think it feels overly complex (needing to now deal with edge cases with copied text) and so maybe not a great primary solution. For example, if I had previously copied something unrelated into the clipboard, I would not want it automatically pasted into the field (eg. the string "Hi Ronald McDonald!"). Then, if you're presenting the user with a choice of pasting in the copied text, that's an entirely new UX and UI that needs to be considered.

@akshayasalvi
Copy link
Contributor

@tgolen Yeah. On it.

@tgolen
Copy link
Contributor

tgolen commented Dec 3, 2021

Cool, proposal looks good to me (though I assume the code will be changed to match our style better in the PR).

I personally don't think that TextInputWithoutKeyboard is necessary at this time, so I think we can do without it for now.

@Christinadobrzyn can you please hire @akshayasalvi for this?

@akshayasalvi
Copy link
Contributor

code will be changed to match our style better in the PR

Yeah I just did a quick test run in a small RN app.

@Christinadobrzyn
Copy link
Contributor

Hired @akshayasalvi in Upwork 🎉

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 6, 2021
@MelvinBot
Copy link

📣 @akshayasalvi You have been assigned to this job by @Christinadobrzyn!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@Christinadobrzyn
Copy link
Contributor

Hi @akshayasalvi! Do you have an update for us on when we can expect the PR for this one? I don't see it yet but maybe I'm missing something. Thank you!

@MelvinBot MelvinBot removed the Overdue label Dec 15, 2021
@akshayasalvi
Copy link
Contributor

@Christinadobrzyn Testing and pushing today.

@tgolen
Copy link
Contributor

tgolen commented Dec 27, 2021

PR has been submitted and is under review.

@MelvinBot MelvinBot removed the Overdue label Dec 27, 2021
@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 4, 2022
@botify botify changed the title Mobile apps - User is unable to paste an amount in the IOU amount modal [HOLD for payment 2022-01-11] Mobile apps - User is unable to paste an amount in the IOU amount modal Jan 4, 2022
@botify
Copy link

botify commented Jan 4, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.24-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-01-11. 🎊

@akshayasalvi
Copy link
Contributor

@parasharrajat @tgolen There's an issue with this PR and I need some help. #7023 linked issue shows two keyboards in Android Chrome.

It seems that inputmode is not working as expected with keyboardType for react-native-web.TextInput component. There is no handling for inputMode, and our inputmode/inputMode gets ignored.

https://github.com/necolas/react-native-web/blob/081a128db325f9ee10c359e304e9f6cc7f4f9c56/packages/react-native-web/src/exports/TextInput/index.js#L136-L161

How should we proceed with this? Should we be updating react-native-web

@parasharrajat
Copy link
Member

Sorry, I was not a reviewer of the PR. inputMode is a browser attribute an RN-web only supports props that are part of native RN spec. So you would have to use setNativeProps to set this. It can't be passed as inputMode prop. Try that and let me know if that does not work.

@akshayasalvi
Copy link
Contributor

I tried on the emulator it isn't working. I'll try to test on a real device.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Jan 13, 2022

Hi there! @parasharrajat sorry to ask but can you give me an update on paying this? We're past the original Hold of Payment date but it looks like there might be other PRs in the works? Can you link the PR to track for paying this (if it's different). Thanks!

@parasharrajat
Copy link
Member

@Christinadobrzyn You should track #6811 for payment.

@Christinadobrzyn
Copy link
Contributor

After 7 days in production without any regressions so paid @akshayasalvi in Upwork for the job!

@parasharrajat were you the C+ on this project?

@parasharrajat
Copy link
Member

@Christinadobrzyn No, but I helped in finding the right solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants