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.36k stars 2.79k forks source link

[Hold for #38025] [$1000] Chat - Email id not shown in full available space & after sending full email iID shown for public accounts #35725

Open kbecciv opened 8 months ago

kbecciv commented 8 months 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!


Version Number: 1.4.36 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

  1. Launch app
  2. Tap in a report
  3. Enter @ in compose box
  4. Select an user from suggestions list
  5. Note email id is not displayed occupying full space
  6. Send the message

Expected Result:

After selecting an email ID from the user suggestion list, it should be displayed in full within the available space. Once sent, the full email ID should not be displayed.

Actual Result:

The selected email id from user suggestion list is not shown in full available space and after sending full email id is displayed.The selected email ID from the user suggestion list is not displayed in full within the available space. Additionally, after sending, the full email ID is still displayed

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/b78b686a-1d83-4555-a1eb-daf122688972

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015976cfa3c5a4e481
  • Upwork Job ID: 1753528802729029632
  • Last Price Increase: 2024-03-07
melvin-bot[bot] commented 8 months ago

Job added to Upwork: https://www.upwork.com/jobs/~015976cfa3c5a4e481

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

kbecciv commented 8 months ago

We think that this bug might be related to #vip-vsb CC @quinthar

MitchExpensify commented 8 months ago

I am having trouble understanding this bug. Can you please rephrase the expected and actual results @kbecciv?

Santhosh-Sellavel commented 7 months ago
Screenshot 2024-02-07 at 2 15 34 AM

@MitchExpensify Check the above screenshot taken from the issue description.

Bug 1

In the composer box email is broken into multiple lines due to a long email, but still, some space on the right side is there.

The same email uses the full available space after sending it in the message area

Screenshot 2024-02-07 at 2 18 06 AM

Bug 2

Additionally, after sending, the full email ID is still displayed

I don't get this properly. But I guess After selecting a user for mentioning from the suggested options, the message should have had the user name in the message instead of the email ID if the user name is available.

Example @Mitch instead mitch@expensify.com

Can you confirm @kbecciv ?

kbecciv commented 7 months ago

Yes, correct! @Santhosh-Sellavel

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

@MitchExpensify, @Santhosh-Sellavel Huh... This is 4 days overdue. Who can take care of this?

Santhosh-Sellavel commented 7 months ago

Bump @MitchExpensify

Santhosh-Sellavel commented 7 months ago

We are still waiting for a confirmation from @MitchExpensify to move forward and proposals as well. Unassigning as am planning for OOO, please assign a new C+ once we decide to move forward!

MitchExpensify commented 7 months ago

Ok got it! Yes lets fix this. I think it could fit in #vip-vsb cc @quinthar

melvin-bot[bot] commented 7 months ago

@MitchExpensify 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 7 months ago

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

Victor-Nyagudi commented 7 months ago

I could reproduce this yesterday, but I'm no longer able to after merging main, even though I'm not entirely confident doing so is what fixed it.

I'll explain why in a little bit.

Firstly, everything worked fine to begin with on Android Web.

Android Chrome screenshots ![Screenshot_20240219_105406_Chrome.jpg](https://github.com/Expensify/App/assets/79470910/1227a0b0-0555-46b6-968c-d864c799e7a8) ![Screenshot_20240219_105529_Chrome.jpg](https://github.com/Expensify/App/assets/79470910/47afaabc-b83b-4d48-9efb-2b1804dffed3) ![Screenshot_20240219_105157_Chrome.jpg](https://github.com/Expensify/App/assets/79470910/c5b7ef7b-c745-483e-98f6-ac1d97b9aca3)

Secondly, I noticed that the reason the text got formatted incorrectly is that the input started with an @ sign followed by some characters that extended to the next line and then a period in between.

This is what happens when you mention a user with a very long email address.

Here's the video I recorded yesterday.

Previous bug recording https://github.com/Expensify/App/assets/79470910/6dbdb959-b659-4e1b-86b0-79525ec7b891

I did some digging and decided to test this assumption using the React Native docs' Expo playground a couple of minutes ago.

The results confirmed my suspicions, and in addition, the bug occurred with a couple of other characters too.

Expo playground results https://github.com/Expensify/App/assets/79470910/a8d11086-311b-4261-9bbe-99fb01da644c

I checked the React Native issues but didn't find anything reported on this behavior.

I believe this could be a bug in the TextInput on Android.

I haven't opened an issue yet on the React Native repo.

melvin-bot[bot] commented 7 months ago

@MitchExpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 7 months ago

@MitchExpensify Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 7 months ago

@MitchExpensify 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 7 months ago

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

MitchExpensify commented 7 months ago

Thanks for digging into the issue @Victor-Nyagudi ! Going to add this as LOW to vip vsb while we wait on proposals

MitchExpensify commented 7 months ago

Switching to Weekly as this is LOW

melvin-bot[bot] commented 7 months ago

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

Thanks!

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @shubham1206agra (Internal)

melvin-bot[bot] commented 7 months ago

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

MitchExpensify commented 7 months ago

@shubham1206agra I notice this is labelled Internal, double checking if you think this can be made External or not

shubham1206agra commented 7 months ago

I believe this can be external. There were no proposals earlier, so we might want to double the bounty to get more eyes on this.

melvin-bot[bot] commented 6 months ago

Upwork job price has been updated to $1000

melvin-bot[bot] commented 6 months ago

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

MitchExpensify commented 6 months ago

Highlighting for proposals here https://expensify.slack.com/archives/C01GTK53T8Q/p1710107159468519

shubham1206agra commented 6 months ago

Marking @tomekzaw @robertKozik to check if this can be fixed with live markdown preview changes. Related PR which might help to fix this https://github.com/Expensify/App/pull/35557

shubham1206agra commented 6 months ago

@tienifr Would you like to take a look at this? You might be able to find some solution.

brandonhenry commented 6 months ago

I'm not able to reproduce

tienifr commented 6 months ago

@shubham1206agra Based on the comment here, there are 2 bugs.

I cannot reproduce Bug 1:

I don't think the second one is a bug because we have always been showing the email, not the full name in mentions.

shubham1206agra commented 6 months ago

@tienifr @tomekzaw See this image

tienifr commented 6 months ago

@shubham1206agra Reproduced on production but not on main.

rojiphil commented 6 months ago

@shubham1206agra I also could not reproduce the second part of the bug in main. But, I could reproduce the first part of the bug. Looking into this a little more closely.

rojiphil commented 6 months ago

Proposal

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

E-mail id is not shown in full available space as shown in the screenshot below

Screenshot 2024-03-11 at 5 39 20 PM

The second part of the issue (i.e. after sending email id) seems to be resolved in the latest main.

What is the root cause of that problem?

The root cause is that we are not supporting a horizontal scroll view for the suggestions view.

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

We can introduce support for horizontal scrolling so that e-mail id can be shown in the available space. However, the implementation is little tricky as FlashList used here for suggestions do not support horizontal scroll. To solve this, we can wrap FlashList with a scrollview to add horizontal support like this here.

  +          <ScrollView 
  +              horizontal 
  +              showsHorizontalScrollIndicator={true}>
            <ColorSchemeWrapper>
                <FlashList
                …
  +                  horizontal={false}
  +                  showsHorizontalScrollIndicator={false}         
                …           
                />
            </ColorSchemeWrapper>
  +          </ScrollView>

However, just setting the horizontal scroll would not be enough as FlashList list size depends on estimatedListSize here. Since the width of estimatedListSize is set here to the available view width, we need to increase the estimated width to accommodate the maximum needed item width.

This maximum estimated item width can be passed into BaseAutoCompleteSuggestions after calculations within MentionSuggestions. As mentioned in the documentation here, we just need a best estimate as a lot of internal tolerance is already incorporated. We can make the below-mentioned code changes which attempt to get the best estimate based on the max number of characters. We can consider other options if needed.

const maxEstimatedMentionWidth = useMemo(
    () => {
        return mentions.reduce((maxEstimatedItemWidth, item) => {
            const styledDisplayNameCount = getStyledTextArray(item.text, prefix).reduce((totalWidth , item) => {return Boolean(item.text) ? totalWidth + item.text.length : 0},0);
            const styledHandleCount = item.text === item.alternateText ? 0 : getStyledTextArray(item.alternateText, prefix).reduce((totalWidth , item) => {return Boolean(item.text) ? totalWidth + item.text.length : 0},0); 
            const totalEstimatedItemWidth = (styledDisplayNameCount + styledHandleCount)*9 + styles.mentionSuggestionsAvatarContainer.width;
            return maxEstimatedItemWidth > totalEstimatedItemWidth ? maxEstimatedItemWidth : totalEstimatedItemWidth;
        },0);
    }, 
    [
        mentions.length,
        prefix,
        styles.mentionSuggestionsAvatarContainer,
    ],
);

Similar implementation needs to be done for EmojiSuggestions

Now, within BaseAutoCompleteSuggestions, we can use the available maxEstimatedMentionWidth to set the estimated width for FlashList so that the entire content of the item can be displayed. We can make the following changes here like this:

    const availableViewWidth = (isLargeScreenWidth ? windowWidth - variables.sideBarWidth : windowWidth) - CONST.CHAT_FOOTER_HORIZONTAL_PADDING;
    const estimatedListSize = useMemo(
        () => ({
            height: CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTION_ROW_HEIGHT * suggestions.length,
            width: maxEstimatedMentionWidth > availableViewWidth ? maxEstimatedMentionWidth : availableViewWidth,
        }),
        [isLargeScreenWidth, suggestions.length, windowWidth, maxEstimatedMentionWidth, isHorizontalScrollingRequired, availableViewWidth],
    );
Test Video https://github.com/Expensify/App/assets/3004659/37488a7e-bfad-40e1-b7ea-a2be7bfb7005

What alternative solutions did you explore? (Optional)

shubham1206agra commented 6 months ago

@rojiphil I think you misunderstood the problem here.

rojiphil commented 6 months ago

I think you misunderstood the problem here.

Oh Silly me! What a waste of my energy here 😓 . Hmmm. But this could be a potential improvement. Will post this in Slack and see if people like it.

rojiphil commented 6 months ago

Proposal

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

E-mail id is not shown in full available space as shown in the screenshot below

Screenshot 2024-03-12 at 10 58 00 AM

The second part of the issue (i.e. after sending email id) seems to be resolved in the latest main.

What is the root cause of that problem?

Here we set the styles for Composer using textInputCompose which sets the horizontal padding to 12 here. However, for smaller screens, real estate is very precious and the current value of horizontal padding of 12 waste away precious space which can be otherwise used for display of text. This seems to be the root cause of this problem.

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

Since there is already a padding of 10 set for the neighbouring emoji, we can reduce the horizontal padding set for Composer. Also, since the current vertical padding of 5 seems good enough for small screen devices, we can set the horizontal padding also to 5 and save some precious real estate for smaller devices.

For this, we can add a style like this here

    smallTextInputComposeHorizontalPadding: {
        paddingHorizontal: variables.avatarChatSpacing - 7,
    },

And use this style for smaller screens like this for Composer here

                style={[styles.textInputCompose, isSmallScreenWidth && styles.smallTextInputComposeHorizontalPadding, isComposerFullSize ? styles.textInputFullCompose : styles.textInputCollapseCompose]}

Similar style change can also be made to ReportActionItemMessageEdit

Here is a comparison of how it will look before and after:

Screenshot 2024-03-12 at 11 22 52 AM

What alternative solutions did you explore? (Optional)

Alternatively, we can reduce the horizontal padding for all screen sizes if required. Also, if needed, we can further reduce the padding.

rojiphil commented 6 months ago

@shubham1206agra What do you think of this proposal? And I hope I am not way off here.

shubham1206agra commented 6 months ago

@rojiphil, I think the issue is related to line breaks happening at @ and .

See for reference

https://github.com/Expensify/App/blob/964548a2adb3ed9e2572ad33456759176a01b76c/src/libs/EmailUtils.ts#L10-L28

rojiphil commented 6 months ago

I think the issue is related to line breaks happening at @ and .

@shubham1206agra That is not seen in the latest main. So, I am confused. Do you think there is still a pending issue to be resolved? Or it is all good here.

shubham1206agra commented 6 months ago

I am able to repro the issue on production. So there's an issue, but maybe it's on production builds only.

shubham1206agra commented 6 months ago

@rojiphil Would you like to test the same on the release build? I can repro the issue on https://github.com/Expensify/App/pull/38081#issuecomment-1989304287 too.

rojiphil commented 6 months ago

Sure. Will check on this.

melvin-bot[bot] commented 6 months ago

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

MitchExpensify commented 6 months ago

Any update @rojiphil ?

rojiphil commented 6 months ago

Sorry for the delay in response here. @shubham1206agra I was able to reproduce the issue. And it looked like a nasty bug and I am nowhere closer to solving this.

shubham1206agra commented 6 months ago

@MitchExpensify Can we get expert teams to tackle this issue?