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.56k stars 2.9k forks source link

[$500] IOU -In BNP page the currency symbol alone shown for a millisecond without amount #26629

Closed lanitochka17 closed 1 year ago

lanitochka17 commented 1 year ago

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. Go to https://staging.new.expensify.com/
  2. Tap Fab
  3. Tap Split bill
  4. Enter an amount
  5. Tap Next
  6. Tap Back

Expected Result:

In split bill, BNP page, the currency symbol with amount must be shown when user tap back

Actual Result:

The user can see in split bill BNP page, the currency symbol alone for a millisecond and then only shows entered amount

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.62-1

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/78819774/2e3ed1d0-812a-4e86-ae48-a370b3b8bb85

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e3c4fda715422964
  • Upwork Job ID: 1699326274642403328
  • Last Price Increase: 2023-09-20
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01e3c4fda715422964

melvin-bot[bot] commented 1 year ago

Current assignee @greg-schroeder is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

greg-schroeder commented 1 year ago

Applied External

melvin-bot[bot] commented 1 year ago

📣 @h0u554m! 📣 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. 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.
  2. 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.
  3. 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>
h0u554m commented 1 year ago

Your Expensify account email: https://new.expensify.com/a/15606409 Upwork Profile Link: https://www.upwork.com/freelancers/~01d68a22e5d906bca6

h0u554m commented 1 year ago

To improve the user experience, you can implement a loading mechanism where you check if the state is not empty. If it's not empty, you display the data. If it is empty, you show a loading indicator, and after a brief delay (e.g., 0.5 seconds or less), you can display a default value. This way, users are aware that data is being fetched, and they eventually see a default value if the data retrieval takes longer.

sobitneupane commented 1 year ago

@h0u554m Can you please put your proposal in proper format. You can go through contributing guidelines and proposal in other issues for more details.

h0u554m commented 1 year ago

To improve the user experience, you can implement a loading mechanism where you check if the state is not empty. If it's not empty, you display the data. If it is empty, you show a loading indicator, and after a brief delay (e.g., 0.5 seconds or less), you can display a default value. This way, users are aware that data is being fetched, and they eventually see a default value if the data retrieval takes longer.

Your Expensify account email: houssammarrakchimk@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01d68a22e5d906bca6

h0u554m commented 1 year ago

@sobitneupane

sobitneupane commented 1 year ago

@h0u554m Can you please post your proposal in this format

h0u554m commented 1 year ago

Problem Statement: Let's clarify the issue we're addressing here. The problem is displaying currency without a number for values less than 1 second, followed by displaying the number.

Root Cause Analysis: The root cause of this issue is that the number is retrieved from the state, and it takes some time to fetch the number and display it alongside the currency.

Proposed Solution: To resolve this issue, we should implement a loading state that waits until the entire page is ready to display the currency and number together.

Alternative Solutions Explored (Optional): Other potential solutions could involve saving the state in local storage or reverting to the default state, i.e., displaying 0, when the number retrieval takes time.

h0u554m commented 1 year ago

@sobitneupane

sobitneupane commented 1 year ago

@h0u554m https://github.com/Expensify/App/issues/25727#issuecomment-1691776494 is a sample of expected proposal. You should link the file(permalink) where the issue exists. Why the line/ piece of code cause the issue? What is your proposed solution. Where will you perform the change? How will it solve the issue.

s-alves10 commented 1 year ago

Proposal

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

The currency symbol alone shown for a millisecond without amount when back to amount page

What is the root cause of that problem?

We're using AmountTextInput in MoneyRequestAmountForm. As you can see below, autoGrow props is true and so TextInput component shows the input element after measures the size of the content. https://github.com/Expensify/App/blob/2ed276bd562f183c0cb39582c73f9e19cda75db4/src/components/AmountTextInput.js#L45

This is the root cause

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

For this simple TextInput element, we can use TextInput from react-native In order to measure content, introduce new state

    const [size, setSize] = useState({width: 0, height: 0});
    const inputStyle = [styles.iouAmountTextInput, styles.p0, styles.noLeftBorderRadius, styles.noRightBorderRadius];

    const isContentMeasured = size.width > 0 && size.height > 0;
    const visibleStyles = isContentMeasured ? [styles.hiddenElementOutsideOfWindow, styles.visibilityHidden] : [];

Show Text before size is measured, and then show TextInput

    return (
        <>
            {isContentMeasured && (
                <RNTextInput
                    showSoftInputOnFocus={false}
                    style={[...inputStyle, size]}
                    onChangeText={props.onChangeAmount}
                    value={props.formattedAmount}
                    placeholder={props.placeholder}
                    placeholderTextColor={themeColors.placeholderText}
                    keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
                    blurOnSubmit={false}
                    selection={props.selection}
                    onSelectionChange={props.onSelectionChange}
                    accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
                    onKeyPress={props.onKeyPress}
                    onFocus={() => Keyboard.dismiss()}
                    ref={(el) => {
                        textInputRef.current = el;
                        if (!props.forwardedRef) {
                            return;
                        }

                        if (_.isFunction(props.forwardedRef)) {
                            props.forwardedRef(el);
                            return;
                        }

                        // eslint-disable-next-line no-param-reassign
                        props.forwardedRef.current = el;
                    }}
                />
            )}
            <Text
                style={[...inputStyle, ...visibleStyles]}
                onLayout={(e) => setSize(e.nativeEvent.layout)}
            >
                {props.formattedAmount || props.placeholder}
            </Text>
        </>
    );

In order to prevent showing keyboard, introduce new ref

    const textInputRef = useRef(null);
    useEffect(() => {
        const appStateSubscription = AppState.addEventListener('change', (nextAppState) => {
            if (!nextAppState.match(/inactive|background/)) {
                return;
            }

            Keyboard.dismiss();
        });

        return () => {
            appStateSubscription.remove();
        };
    }, []);

    useEffect(() => {
        if (!textInputRef.current || !textInputRef.current.setAttribute || !isContentMeasured) {
            return;
        }
        textInputRef.current.setAttribute('inputmode', 'none');
    }, [isContentMeasured])

This works as expected

Result https://github.com/Expensify/App/assets/126839717/85455549-fc96-4bbb-bf92-1e9d94bc2532

What alternative solutions did you explore? (Optional)

We can create a new component SimpleTextInput for AmountTextInput

melvin-bot[bot] commented 1 year 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 year ago

@sobitneupane @greg-schroeder 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!

melvin-bot[bot] commented 1 year ago

@sobitneupane, @greg-schroeder Huh... This is 4 days overdue. Who can take care of this?

greg-schroeder commented 1 year ago

awaiting proposal review

sobitneupane commented 1 year ago

@s-alves10 Can you confirm if this problem can still be replicated? I'm not able to reproduce it on my side.

https://github.com/Expensify/App/assets/25876548/9718d25c-d018-4ac1-8e1c-4a3e9a6efa93

s-alves10 commented 1 year ago

@sobitneupane

Yes, this is still reproducible. This issue happens only for split bill, not request money. Please try to test in a group chat

melvin-bot[bot] commented 1 year ago

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

sobitneupane commented 1 year ago

@s-alves10 I couldn't reproduce the issue with the split bill either.

https://github.com/Expensify/App/assets/25876548/3e11c59f-0789-406c-b452-623f22c08da5

s-alves10 commented 1 year ago

@sobitneupane

I can see the bug in your recorded video when navigating back from selecting currency page

melvin-bot[bot] commented 1 year ago

@sobitneupane @greg-schroeder this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

greg-schroeder commented 1 year ago

Hmm.. maybe I'm not seeing it. At one timestamp in the video do you see the bug @s-alves10?

s-alves10 commented 1 year ago

I can see the bug between 13s ~ 14s

sobitneupane commented 1 year ago

Thanks @s-alves10 I finally noticed the issue. 😀

It exists for only a fraction of a second. It is almost imperceptible. I don't think it is worth fixing.

cc: @greg-schroeder

greg-schroeder commented 1 year ago

I had the timestamp and knew what the bug was and it still took me like 60 seconds to see it. Honestly I tend to agree. I'm going to close this.