Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.33k stars 2.76k forks source link

[$250] [POLISH][Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input #47875

Open rayane-djouah opened 3 weeks ago

rayane-djouah commented 3 weeks ago

This is a follow-up issue. Coming from: https://github.com/Expensify/App/pull/47343#issuecomment-2291467068 and https://github.com/Expensify/App/pull/47343#issuecomment-2298616516

Bug in iOS and Android native apps: When entering a number, the digit exceeding the maximum allowed decimal places is briefly inserted into the input field before being deleted. They should not be inserted at all.

https://github.com/user-attachments/assets/5a9be667-53e0-4dc5-bf95-132ad6f09cca

cc @luacmartins @289Adam289

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017b65a6e5c7b77ba2
  • Upwork Job ID: 1827006416140957560
  • Last Price Increase: 2024-09-13
Issue OwnerCurrent Issue Owner: @brunovjk
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 3 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~017b65a6e5c7b77ba2

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @brunovjk (External)

eyobai commented 3 weeks ago

Validation Function: Create a function to check if the input exceeds the allowed decimal places. Event Listener: Use the input event to validate each change to the input field. Immediate Correction: If the input is invalid, immediately revert to the last valid state.

melvin-bot[bot] commented 3 weeks ago

📣 @eyobai! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
huult commented 3 weeks ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The digit exceeding the max allowed decimal places is briefly inserted into the input

What is the root cause of that problem?

This issue occurs because we use onChangeText to validate the amount between lines 37 and line 40. If the amount is not valid, we return and remove the text change in the text input

https://github.com/Expensify/App/blob/0c8455280c738a5db596f34409a0a3177e682e7f/src/components/AmountWithoutCurrencyForm.tsx#L29-L43

What changes do you think we should make in order to solve the problem?

We should check that the decimal limit has been reached and no more decimals can be entered before the text changes.

// src/components/AmountWithoutCurrencyForm.tsx#L17
+ const decimals = 2;

// src/components/AmountWithoutCurrencyForm.tsx#L37
- if (!validateAmount(withLeadingZero, 2)) {
+ if (!validateAmount(withLeadingZero, decimals)) {

// src/components/AmountWithoutCurrencyForm.tsx#L45
+     const getDecimalLength = (numStr: string) => {
+        const parts = numStr.split('.');
+        return parts[1] ? parts[1].length : 0;
+    };

// src/components/AmountWithoutCurrencyForm.tsx#L46
    return (
        <TextInput
            value={formattedAmount}
            onChangeText={setNewAmount}
            inputID={inputID}
            name={name}
            label={label}
            defaultValue={defaultValue}
            accessibilityLabel={accessibilityLabel}
            role={role}
            ref={ref}
            keyboardType={CONST.KEYBOARD_TYPE.DECIMAL_PAD}
+           maxLength={getDecimalLength(formattedAmount) === decimals ? formattedAmount.length : undefined}
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...rest}
        />
    );
POC https://github.com/user-attachments/assets/112bc5dd-67bf-447b-8f6b-e682b7555826
brunovjk commented 3 weeks ago

Thank you for the proposal @huult, but I tested it and it doesn't seem to resolve the issue. The RCA focuses on maxLength, but the problem occurs only on native platforms. If you believe your solution fixes this, please share a test branch for review. For context, I just reproduced the issue on web and native. On web, typing 1111111111111111111111.111111111111 shows 11111111.11 without issue. However, on native (I only tested on iOS so far), the number stays as 1111111111111111111111.111111111111 for a couple of seconds before correcting to 11111111.11. The issue may require further investigation into why native platforms are behaving differently.

huult commented 2 weeks ago

@brunovjk , Thanks for reviewing my proposal. I double-checked and found that it works when the user types from the keyboard but does not work when pasting a number into the input field or entering multiple decimal points. I am investigating further.

AlfredoAlc commented 2 weeks ago

This most likely is related to a known issue for the TextInput component from react-native, specially for iOS platforms. See this issue for reference: https://github.com/facebook/react-native/issues/36494

melvin-bot[bot] commented 2 weeks ago

@luacmartins, @isabelastisser, @brunovjk Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

brunovjk commented 2 weeks ago

Thank you, @AlfredoAlc, for the reference. It aligns with our issue, but I haven’t found a fix yet, only workarounds. @huult, your proposal appears reasonable as it directly enforces decimal limits. However, despite the straightforward nature of the suggested changes, they haven’t resolved the issue in my tests. Do you have any new findings or a test branch to share? Thank you all!

huult commented 2 weeks ago

@brunovjk , test branch for my proposal

POC https://github.com/user-attachments/assets/f71a1978-2707-4848-ac37-b5ad217bc950
brunovjk commented 2 weeks ago

@brunovjk , test branch for my proposal

Thank you @huult :D I just tested. Almost there, but the problem is not just in the decimals:

https://github.com/user-attachments/assets/a8bd49c4-8524-41a3-b0f6-c9cd07d4482e

If the video above is no longer available when you watch it, let me know.

huult commented 2 weeks ago

@brunovjk , I've updated the test branch to fix the issue you tested.

https://github.com/user-attachments/assets/de216d0d-f694-4ed1-a797-7321e788afcf

brunovjk commented 2 weeks ago

Thank you for your efforts, @huult. Unfortunately, the solution still isn’t working as expected on my end. As we can't attack the root cause directly, we might need a "workaround" to address this effectively. I’m confident we can find one that won’t introduce regressions. Let’s keep exploring options—I’ll revisit this next week, and we can involve an internal engineer if necessary.

https://github.com/user-attachments/assets/756527da-904a-4cf6-8d91-0558d03308e2

melvin-bot[bot] commented 2 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 week ago

@luacmartins, @isabelastisser, @brunovjk Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

brunovjk commented 1 week ago

Update: Posted a request for help on this issue in #contributor-plus to get more insights.

isabelastisser commented 1 week ago

Thanks for the update!

melvin-bot[bot] commented 1 week ago

@luacmartins @isabelastisser @brunovjk this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

brunovjk commented 1 week ago

Hey @isabelastisser, @luacmartins, this issue seems to stem from React Native’s handling of inputs on mobile, https://github.com/Expensify/App/issues/47875#issuecomment-2311234992. I suggest we raise compensation to attract more attention, especially for potential fixes. Thoughts on how to proceed? Thanks!

luacmartins commented 1 week ago

I think we should keep it as is given it's not a critical issue

melvin-bot[bot] commented 1 week ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Nodebrute commented 1 week ago

@luacmartins @brunovjk Hi, I just wanted to point out that this issue isn't limited to the search fields. It's also occurring in other fields, such as the Max-Age and the Max-Amount fields. Additionally, even when pressing the alphabet keys, the characters briefly appear before disappearing so the issue is not just maxLength

https://github.com/user-attachments/assets/485052c6-d995-439e-8b5c-8d8854603d29

melvin-bot[bot] commented 6 days ago

@luacmartins, @isabelastisser, @brunovjk Whoops! This issue is 2 days overdue. Let's get this updated quick!

brunovjk commented 6 days ago

Not overdue, still looking for proposals.

melvin-bot[bot] commented 2 days ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸