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 2021-11-11] Chat - "Typing" isn't visible when someone from another account has longer email is typing #5623

Closed
kavimuru opened this issue Oct 1, 2021 · 33 comments · Fixed by #5933
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Oct 1, 2021

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. Lunch the app
  2. Log in with any account ( Account A)
  3. Log in with another account where you don't have any last and first name to sets up ( Account B)
  4. Start typing from Account B to Account A

Expected Result:

"Typing" is visible when someone from another account has longer email without name is typing

Actual Result:

"Typing" isn't visible when someone from another account has longer email without name is typing

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android
  • Mobile Web

Version Number: 1.1.4.0
Reproducible in staging?: Yes
**Reproducible in production?:**Yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Bug5260547_Screenshot_20210930-192721_Gallery__1_

Expensify/Expensify Issue URL:
Issue reported by: Applause
Slack conversation:

View all open jobs on GitHub

@MelvinBot
Copy link

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

@PrashantMangukiya
Copy link
Contributor

Proposed Solution (If consider External)

We need to update code within src/pages/home/report/ReportTypingIndicator.js
i..e Add ellipsizeMode="middle" for Text component i..e After line 62, 77, and 94 , So it shows ... in between long text.

<Text
style={[
styles.chatItemComposeSecondaryRowSubText,
styles.chatItemComposeSecondaryRowOffset,
]}
numberOfLines={1}
>
{getDisplayName(this.state.usersTyping[0])}
{` ${this.props.translate('reportTypingIndicator.isTyping')}`}
</Text>

<Text
style={[
styles.chatItemComposeSecondaryRowSubText,
styles.chatItemComposeSecondaryRowOffset,
]}
numberOfLines={1}
>
{getDisplayName(this.state.usersTyping[0])}
{` ${this.props.translate('common.and')} `}
{getDisplayName(this.state.usersTyping[1])}
{` ${this.props.translate('reportTypingIndicator.areTyping')}`}
</Text>

<Text
style={[
styles.chatItemComposeSecondaryRowSubText,
styles.chatItemComposeSecondaryRowOffset,
]}
numberOfLines={1}
>
{this.props.translate('reportTypingIndicator.multipleUsers')}
{` ${this.props.translate('reportTypingIndicator.areTyping')}`}
</Text>

i.e. Add add ellipsizeMode="middle" at three places i..e After line 62 (for Single User), 77 (for Two User), and 94 (for More than two user) as shown under.

<Text 
     ...
     numberOfLines={1} 
     ellipsizeMode="middle"       // *** Add this line
 > 
     {getDisplayName(this.state.usersTyping[0])} 
     {` ${this.props.translate('reportTypingIndicator.isTyping')}`} 
 </Text> 

<Text 
    ...
     numberOfLines={1} 
     ellipsizeMode="middle"     // *** Add this line
 > 
     {getDisplayName(this.state.usersTyping[0])} 
     {` ${this.props.translate('common.and')} `} 
     {getDisplayName(this.state.usersTyping[1])} 
     {` ${this.props.translate('reportTypingIndicator.areTyping')}`} 
 </Text> 

<Text 
    ...
     numberOfLines={1} 
     ellipsizeMode="middle"     // *** Add this line
 > 
     {this.props.translate('reportTypingIndicator.multipleUsers')} 
     {` ${this.props.translate('reportTypingIndicator.areTyping')}`} 
 </Text> 

Important: ellipsizeMode="middle" works for IOS and Android as shown under. But Not Support in Web, Mobile Web and Desktop.

iOS

iOS

Android

Android

@timszot timszot removed their assignment Oct 1, 2021
@timszot timszot added the External Added to denote the issue can be worked on by a contributor label Oct 1, 2021
@MelvinBot
Copy link

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

@timszot timszot added Weekly KSv2 and removed Daily KSv2 labels Oct 1, 2021
@isagoico
Copy link

isagoico commented Oct 4, 2021

Issue reproducible during KI retests

@bfitzexpensify
Copy link
Contributor

Upwork posting

@akshayasalvi
Copy link
Contributor

Proposal

Because ellipsizeMode doesn't work for 3 of 5 platforms, my recommendation would be, let's split the content into their individual Text blocks and use flex: row.

An example implementation:

<Text
    style={[
        styles.chatItemComposeSecondaryRowSubText,
        styles.chatItemComposeSecondaryRowOffset,
    ]}
    numberOfLines={1}
>
    {getDisplayName(this.state.usersTyping[0])}
</Text>
<Text
    style={[
        styles.chatItemComposeSecondaryRowSubText,
        styles.chatItemComposeSecondaryRowOffset,
    ]}
    numberOfLines={1}
>
    {` ${this.props.translate('common.and')} `}
</Text>
<Text
    style={[
        styles.chatItemComposeSecondaryRowSubText,
        styles.chatItemComposeSecondaryRowOffset,
    ]}
    numberOfLines={1}
>
    {getDisplayName(this.state.usersTyping[1])}
</Text>
<Text
    style={[
        styles.chatItemComposeSecondaryRowSubText,
        styles.chatItemComposeSecondaryRowOffset,
    ]}
    numberOfLines={1}
>
    {` ${this.props.translate('reportTypingIndicator.areTyping')}`}
</Text>

Anyway, each of the content is rendered separately.

We can even reduce the repetition of <Text> with chatItemComposeSecondaryRowSubText and styles.chatItemComposeSecondaryRowOffset styles.

@mvtglobally
Copy link

Issue reproducible during KI retests.

@akshayasalvi
Copy link
Contributor

@bfitzexpensify Any updates on this one?

@bfitzexpensify
Copy link
Contributor

Sorry @akshayasalvi — currentlly looking for an engineer to review the proposal. Hang tight!

@bfitzexpensify
Copy link
Contributor

Hello @NikkiWines, the exported label isn't working so I randomly chose you from the list of engineering contributor managers 😄.

@akshayasalvi has a solution above, so hopefully this one is straightforward. Ty for your understanding

@parasharrajat
Copy link
Member

I have few points to share here and I don't think that the proposed solution would produce the desired result.

Why?

  1. If we divide the text as proposed then we will end up with multiple ... on the row. and still, we won't be able to see which users are messaging (in case of multiple users).

What I propose.

  1. Either we show eg: User A +2 users are typing. + number of users, after the first user name.

  2. Or, We animate the typing string one by one for each user.

It would work like this

 USER A is typing...
 USer B
 USer C

Here User part will fade and the user's name will change one by one.

Thought?

@NikkiWines
Copy link
Contributor

I think I prefer the idea of User A and <x> others are typing as opposed to using ellipsis. However, let's ask someone from the design team to see if there's a preference here.

cc: @Expensify/design

@NikkiWines
Copy link
Contributor

NikkiWines commented Oct 14, 2021

Ah, I see. But then, the issue would also persist for multiple two users if one (or both) of those users had very long logins and both were typing.

@parasharrajat
Copy link
Member

parasharrajat commented Oct 14, 2021

Sorry for dropping the different context in but this issue indirectly affects the scenario where multiple users are typing.

As Nikki said that User A and user B are typing... is better. Then

we can divide Typing Text block into two blocks instead of multiple for each user and other words and etc.

It would be like

<Text > User A and user B</Text> // only ellipsis for this.
<Text> are typing..</Text>

This way we can preserve the are typing... and only one ellipsis.

The other two approaches, I have explained above.

@shawnborton
Copy link
Contributor

Hmm I'm not sure what is best then, @NikkiWines curious for your thoughts. I think we should avoid any kind of cheeky animation though if we can.

@NikkiWines
Copy link
Contributor

I don't know if having an elipsis for User A and User B only would work, since there's a possibility of it resulting in something misleading like:

User+with+super+long+name+or+email and User B are typing...

becoming:

User+with+super+long... are typing

I wonder if maybe we could simplify this. Instead of 3 different potential copies, maybe we could do:

  • 1 user typing:
    User A (elipsis if necessary) is typing
  • 2 or more users typing:
    <x> users are typing or <x> people are typing

Not sure if we have some reasoning for separate copy for 2 users typing vs 3 users typing though

@shawnborton
Copy link
Contributor

I like simplifying this to 2 scenarios! We could even keep it super generic like "Multiple people are typing..." instead of having to worry about updating the count.

@akshayasalvi
Copy link
Contributor

This makes more sense. This can then just be simplified by removing case 2: from ReportTypingIndicator swith case.

and then as I suggested split the content into multiple <Text> block when we have 1 user. This way ellipses can only be added to the User Text block and not the others.

@NikkiWines
Copy link
Contributor

Yep, that makes sense to me! @bfitzexpensify could you hire @akshayasalvi for this issue please, thank you!

@mvtglobally
Copy link

Issue reproducible during KI retests.

@NikkiWines NikkiWines linked a pull request Oct 18, 2021 that will close this issue
5 tasks
@NikkiWines NikkiWines added the Reviewing Has a PR in review label Oct 18, 2021
@parasharrajat
Copy link
Member

Obviously, I am late to the party but...

I will be reviewing the PR and I will let you know @NikkiWines when this is ready for final review and possibly for merge. See More

@mvtglobally
Copy link

Issue reproducible during KI retests.

@mallenexpensify mallenexpensify changed the title Chat - "Typing" isn't visible when someone from another account has longer email is typing [PAY 11/8] Chat - "Typing" isn't visible when someone from another account has longer email is typing Nov 2, 2021
@mallenexpensify
Copy link
Contributor

@bfitzexpensify updated title to pay on 11/8, doesn't look like was auto updated yesterday when the issue closed. Also reopening this til contributor is paid. @NikkiWines let me know if I'm missing anything here

@NikkiWines
Copy link
Contributor

Oh sorry, must've auto-closed cause I merged the PR. Your update looks good @mallenexpensify

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 4, 2021
@botify botify changed the title [PAY 11/8] Chat - "Typing" isn't visible when someone from another account has longer email is typing [HOLD for payment 2021-11-11] [PAY 11/8] Chat - "Typing" isn't visible when someone from another account has longer email is typing Nov 4, 2021
@botify
Copy link

botify commented Nov 4, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.13-2 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 2021-11-11. 🎊

@akshayasalvi
Copy link
Contributor

@NikkiWines @mallenexpensify I am not sure why the payment date changed to Nov 11 from Nov 8. Didn't find any reported regression.

@parasharrajat
Copy link
Member

Payment date is 7 days after deployment to PROD #5933 (comment)

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First Week)

@mallenexpensify mallenexpensify changed the title [HOLD for payment 2021-11-11] [PAY 11/8] Chat - "Typing" isn't visible when someone from another account has longer email is typing [HOLD for payment 2021-11-11] Chat - "Typing" isn't visible when someone from another account has longer email is typing Nov 8, 2021
@bfitzexpensify
Copy link
Contributor

Contract ended, payment finalised - thanks @akshayasalvi!

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 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.