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] Web - Request Money - Long description is showing without "Paid by" and "Split with" #28073

Closed kbecciv closed 1 year ago

kbecciv 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. Click on FAB -> Request money -> Enter any numbers
  2. Select some account -> Add to Split button
  3. Enter a long description -> Save -> Split button
  4. Click on newly Request money -> Observe this Request money

Expected Result:

Request money will display "Paid by" and "Split with" as well as the Description.

Actual Result:

Long description is showing only without "Paid by" and "Split with"

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.73.0 Reproducible in staging?: y Reproducible in production?: y 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/93399543/9ef6c502-fe49-43df-9990-6c27562720bf

screen-capture (1) (2).webm

https://github.com/Expensify/App/assets/93399543/bceb7534-1116-408b-b58e-47b80613fd6e

Expensify/Expensify Issue URL: Issue reported by: @thuyle04 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695272345551449

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01062695b5b87a83cc
  • Upwork Job ID: 1705616890113011712
  • Last Price Increase: 2023-10-14
esh-g commented 1 year ago

Proposal

Please re-state the issue

Split bill details page is not scrollable and is stuck when a long description is provided.

What is the root cause of this issue?

First We are not keeping the MoneyRequestConfirmationList in a ScrollView in SplitBillDetailsPage.js https://github.com/Expensify/App/blob/5aa7f6da94ddb1c8ca55a6cce714555f819c1dd0/src/pages/iou/SplitBillDetailsPage.js#L82-L94

This makes it impossible to scroll and the page seems stuck.

Second The reason that description is not getting truncated despite passing numberOfLines here: https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/components/MoneyRequestConfirmationList.js#L509

is because instead of a Text component, description is being rendered as HTML which we cannot truncate with the numberOfLines prop. https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/components/MenuItem.js#L253-L265

What changes do you think should be made to fix this problem?

First We should nest the MoneyRequestConfirmationList in a double ScrollView like we do in MoneyRequestConfirmPage.js here: https://github.com/Expensify/App/blob/5aa7f6da94ddb1c8ca55a6cce714555f819c1dd0/src/pages/iou/steps/MoneyRequestConfirmPage.js#L290-L300

We should also add the pointerEvents='box-none' to the ScrollView which is currently present on the parent View in SplitBillDetailsPage.

Second According to this comment, In MenuItemRenderHTMLTitle/index.js, we need to limit the description to 2-3 lines unless the show more button is pressed. For that, we can change the MenuItemWithTopDescription in MoneyRequestConfirmationList like this:

<MenuItemWithTopDescription
    shouldShowRightIcon={!props.isReadOnly}
    shouldParseTitle
    title={props.iouComment}
    description={translate('common.description')}
    ...
    numberOfLinesTitle={shouldShowAllFields ? null : 3} // this line changed
/>

This will make sure that the description is limited to only 3 lines unless it is expanded.

Third In MenuItem.js, we need to change the maxHeight to allow only the required lines unless it is expanded. Here: https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/components/MenuItem.js#L253-L256

We can pass change the style like this:

const titleContainerStyle = StyleUtils.combineStyles([
        styles.flexRow,
        styles.alignItemsCenter,
        styles.overflowHidden,
        {maxHeight: props.numberOfLinesTitle ? variables.lineHeightXLarge * props.numberOfLinesTitle : undefined}
    ]);

    ...
<View style={titleContainerStyle}>
    {Boolean(props.title) && (Boolean(props.shouldRenderAsHTML) || (Boolean(props.shouldParseTitle) && Boolean(html.length))) && (
        <MenuItemRenderHTMLTitle title={getProcessedTitle} />
    )}
    ....

Result:

https://github.com/Expensify/App/assets/77237602/d156eaef-6229-4ed2-a971-e4400f9e67bd

What other alternative options did you explore? (Optional)

If we want to set a fixed limit for the number of characters instead of number of lines for the description, we can also use a custom truncate function with a fixed number of words for the description. I prefer the previous solution because that allows flexibility of using numberOfLines for menu items that are using html rendering.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01062695b5b87a83cc

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

newangry commented 1 year ago

Contributor details Your Expencify account email: williamholiday777@gmail.com Upwork Profile Link: https://www.upwork.com/ab/proposals/1705602690487279617?success

Please remove horizontal attribute in ScrollView component.

newangry commented 1 year ago

https://www.upwork.com/freelancers/~019a9ce9f3628775b7 Contributor details Your Expencify account email: williamholiday777@gmail.com Upwork Profile Link: https://www.upwork.com/ab/proposals/1705602690487279617?success

Please remove horizontal attribute in ScrollView component.

anmurali commented 1 year ago

@situchan have you had a chance to reproduce this bug and review proposals?

astrohunter62 commented 1 year ago

Proposal

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

When a lengthy description is entered, the split bill details page becomes unable to scroll.

What is the root cause of that problem?

In the SplitBillDetailsPage, the MoneyRequestConfirmationList is enclosed within a View component. However, in React Native, View components do not enable scrolling.

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

We can opt for the ScrollView component instead of View.

https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/pages/iou/SplitBillDetailsPage.js#L82-L94

We can change this code.

<ScrollView contentContainerStyle={[styles.flexGrow1]}>
    <ScrollView
        horizontal
        contentContainerStyle={[styles.flex1, styles.flexColumn]}
    >
        {Boolean(participants.length) && (
            <MoneyRequestConfirmationList
                hasMultipleParticipants
                payeePersonalDetails={payeePersonalDetails}
                selectedParticipants={participantsExcludingPayee}
                iouAmount={splitAmount}
                iouComment={splitComment}
                iouCurrencyCode={splitCurrency}
                iouType={CONST.IOU.MONEY_REQUEST_TYPE.SPLIT}
                isReadOnly
                shouldShowFooter={false}
                listStyles={[StyleUtils.getMaximumHeight(windowHeight / 3)]}
            />
        )}
    </ScrollView>
</ScrollView>

And https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/components/MoneyRequestConfirmationList.js#L500-L510

<View style={[styles.flexGrow1, styles.overflowHidden, !shouldShowAllFields && styles.autoGrowHeightMultilineInput]}>
    <MenuItemWithTopDescription
        shouldShowRightIcon={!props.isReadOnly}
        shouldParseTitle
        title={props.iouComment}
        description={translate('common.description')}
        onPress={() => Navigation.navigate(ROUTES.MONEY_REQUEST_DESCRIPTION.getRoute(props.iouType, props.reportID))}
        style={[styles.moneyRequestMenuItem]}
        titleStyle={styles.flex1}
        disabled={didConfirm || props.isReadOnly}
        numberOfLinesTitle={2}
    />
</View>

Result:

updated.webm

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

@anmurali, @situchan Eep! 4 days overdue now. Issues have feelings too...

anmurali commented 1 year ago

Bumped @situchan here

melvin-bot[bot] commented 1 year ago

@anmurali @situchan 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

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

situchan commented 1 year ago

I feel like "Show more" button needs to show without scrolling down. So when collapsed, show only a few lines of description and when expanded, show full description, followed by Date and Merchant. cc: @Expensify/design

https://github.com/Expensify/App/assets/108292595/a761fca6-30a7-4f03-871d-9f7fa0faa8db

esh-g commented 1 year ago

@situchan I have updated my proposal accordingly

situchan commented 1 year ago

@esh-g can you share screenshot? The design team will take decision.

astrohunter62 commented 1 year ago

I updated my proposal

esh-g commented 1 year ago

@situchan Here is the screen recording (already added to proposal as result):

Android

https://github.com/Expensify/App/assets/77237602/d156eaef-6229-4ed2-a971-e4400f9e67bd

Web

https://github.com/Expensify/App/assets/77237602/05c919df-f5b6-4cfb-ac63-96be12c902a1

cc @Expensify/design

dubielzyk-expensify commented 1 year ago

The last video looks good to me. It does make me think we should also do truncate the description in the chat as well, not just the details panel.

Thoughts from @dannymcclain and @shawnborton ?

shawnborton commented 1 year ago

My thinking is that for the preview cards, we should truncate after 2-3 lines as the whole point is that it is a preview, and not the full thing.

Then when viewing the actual expense/task, I feel like we shouldn't truncate anything as that's the full view you would expect. Instead, we should be enforcing a certain character limit for things like descriptions, etc.

situchan commented 1 year ago

@shawnborton do you agree this is fine? (no truncation)

https://github.com/Expensify/App/assets/108292595/9ac4d878-474d-4548-a489-f65c86a6eda0

shawnborton commented 1 year ago

Yup, I do agree that it is fine. Again, I think truncation should happen only in previews, and we should enforce character limits in places where we don't want a ton of text.

Either way, what you are showing above is an edge case, so I'd love to actually experience where customers see this kind of edge case first before making rules for it.

situchan commented 1 year ago

@shawnborton thanks for the feedback.

In this case, preview can be out of scope and enable scrolling on split bill detail page is the only one to fix here.

Awaiting proposal that is responsive to all platforms without any console errors

esh-g commented 1 year ago

@situchan Could you please review my proposal? This will not cause any console errors and have tested on all platforms. Feel free to ignore the description truncation logic

youssef-lr commented 1 year ago

Heads up @esh-g I'm currently working on a PR that's gonna make MoneyRequestConfirmationList scrollable as we added errors to the fields and they take up space, it's a bit urgent so we'll try to probably merge it today.

melvin-bot[bot] commented 1 year ago

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

situchan commented 1 year ago

@situchan Could you please review my proposal? This will not cause any console errors and have tested on all platforms. Feel free to ignore the description truncation logic

Your solution will cause console error on native. We should use horizontal scroll view to avoid VirtualizedList warning

situchan commented 1 year ago

https://github.com/Expensify/App/pull/29568 is fixing this issue as well. @youssef-lr I can do expedite review right now if urgent.

esh-g commented 1 year ago

@situchan I mentioned in my proposal to use a "double scrollView" while also attaching the place where we use the horizontal one.

Screenshot 2023-10-13 at 10 07 01 PM
situchan commented 1 year ago

Oh ok, but unfortunately it's being worked internally.

melvin-bot[bot] commented 1 year ago

@anmurali @situchan 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!

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? 💸

youssef-lr commented 1 year ago

This should be fixed here https://github.com/Expensify/App/pull/29568

situchan commented 1 year ago

I think the only remaining thing here is to pay @thuyle04 for the bug report.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@anmurali @situchan this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.

thuyle04 commented 1 year ago

Contributor details Your Expensify account email: thuy.lethithu@antbuddy.com Upwork Profile Link: https://www.upwork.com/freelancers/~01fcd9ed589276332f

melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] commented 1 year ago

@anmurali, @situchan 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

situchan commented 1 year ago

Not Overdue

melvin-bot[bot] commented 1 year ago

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

situchan commented 1 year ago

We can close this