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 2022-11-02] [$250] The ‘Multiple users are typing’ vertical text alignment is incorrect #10486

Closed
mvtglobally opened this issue Aug 23, 2022 · 39 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mvtglobally
Copy link

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. Open group chat as user A
  2. Type as User B on another device
  3. Observe underlying "typing..." display as User A
  4. Type as User B and User C on 2 different devices simultaneously
  5. Observe underlying "typing..." display as User A

Expected Result:

Text should be aligned

Actual Result:

The ‘Multiple users are typing’ vertical text alignment is incorrect. When a single user is typing the text remains centered, but when multiple users are typing the text becomes uncentered:

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web
  • Desktop App

Version Number: 1.1.88-14
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

WebmultipleUsersTyping.mov

Expensify/Expensify Issue URL:
Issue reported by: @Julesssss
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1660833359041749

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2022

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

@Julesssss Julesssss assigned Julesssss and unassigned robertjchen Aug 23, 2022
@Julesssss
Copy link
Contributor

@robertjchen I'll take this as I'm on the C+ team, and reported the issue.

@Julesssss Julesssss added the External Added to denote the issue can be worked on by a contributor label Aug 23, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2022

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

@mdneyazahmad
Copy link
Contributor

Proposal

case 1:
return (
<TextWithEllipsis
leadingText={PersonalDetails.getDisplayName(this.state.usersTyping[0])}
trailingText={` ${this.props.translate('reportTypingIndicator.isTyping')}`}
textStyle={[styles.chatItemComposeSecondaryRowSubText]}
wrapperStyle={[styles.chatItemComposeSecondaryRow]}
leadingTextParentStyle={styles.chatItemComposeSecondaryRowOffset}
/>
);
default:
return (
<Text
style={[
styles.chatItemComposeSecondaryRowSubText,
styles.chatItemComposeSecondaryRowOffset,
]}
numberOfLines={1}
>
{this.props.translate('reportTypingIndicator.multipleUsers')}
{` ${this.props.translate('reportTypingIndicator.areTyping')}`}
</Text>
);

Use same component that is used to display single user typing message.

-                    <Text
-                        style={[
-                            styles.chatItemComposeSecondaryRowSubText,
-                            styles.chatItemComposeSecondaryRowOffset,
-                        ]}
-                        numberOfLines={1}
-                    >
-                        {this.props.translate('reportTypingIndicator.multipleUsers')}
-                        {` ${this.props.translate('reportTypingIndicator.areTyping')}`}
-                    </Text>
+                    <TextWithEllipsis
+                        leadingText={this.props.translate('reportTypingIndicator.multipleUsers')}
+                        trailingText={` ${this.props.translate('reportTypingIndicator.areTyping')}`}
+                        textStyle={[styles.chatItemComposeSecondaryRowSubText]}
+                        wrapperStyle={[styles.chatItemComposeSecondaryRow]}
+                        leadingTextParentStyle={styles.chatItemComposeSecondaryRowOffset}
+                    />

Before

Screenshot 2022-08-23 at 3 13 00 PM

After

Screenshot 2022-08-23 at 3 11 38 PM

@MitchExpensify
Copy link
Contributor

Upwork Job

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 23, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2022

Current assignee @Julesssss is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 23, 2022
@melvin-bot melvin-bot bot changed the title The ‘Multiple users are typing’ vertical text alignment is incorrect [$250] The ‘Multiple users are typing’ vertical text alignment is incorrect Aug 23, 2022
@parasharrajat
Copy link
Member

Checking on the proposal.

@parasharrajat
Copy link
Member

@mdneyazahmad's proposal seems fine to me. The only difference will be that multiple users will start to show ellipsis instead of trimming the are typing... part, when the screen area is less than the required space for multiple users are typing.... This is unlikely to happen.

cc: @Julesssss

🎀 👀 🎀 C+ reviewed

@mdneyazahmad
Copy link
Contributor

@parasharrajat I also think the same atleast in multiple user typing case it is very static "Multiple users are typing..." but when a single user is typing the name could be long and it will overflow the screen and the result will not be what are we expecting. But that is an issue that exist within TextWithEllipsis and is very unlikely to happen.

@parasharrajat
Copy link
Member

that is an issue that exist within TextWithEllipsis

It is not an issue. TextWithEllipsis is created to behave that way.

@mdneyazahmad
Copy link
Contributor

I see, are we fine or need to make some changes in my proposal?

const TextWithEllipsis = props => (
<View style={[styles.flexRow, ...StyleUtils.parseStyleAsArray(props.wrapperStyle)]}>
<View style={[styles.flexShrink1, ...StyleUtils.parseStyleAsArray(props.leadingTextParentStyle)]}>
<Text style={[...StyleUtils.parseStyleAsArray(props.textStyle)]} numberOfLines={1}>
{props.leadingText}
</Text>
</View>
<View style={styles.flexShrink0}>
<Text style={props.textStyle}>
{props.trailingText}
</Text>
</View>
</View>
);

We can do some changes in TextWithEllipsis and make trailingText an optional and conditionally render trailing text.

+        {props.trailingText && (
         <View style={styles.flexShrink0}>
             <Text style={props.textStyle}>
                 {props.trailingText}
             </Text>
         </View>
+        )}

Now, we can pass text combined to leadingText prop.

@parasharrajat
Copy link
Member

No that changes the motive of using TextWithEllipsis. After these changes, the end result can also be achieved using a simple Text component.

That said, your previous proposal is fine.

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2022
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 2, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 2, 2022

📣 @mdneyazahmad You have been assigned to this job by @Julesssss!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added the Overdue label Oct 3, 2022
@Julesssss
Copy link
Contributor

It looks like there's an ongoing discussion in the PR review.

@melvin-bot melvin-bot bot removed the Overdue label Oct 4, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 12, 2022
@Julesssss
Copy link
Contributor

Hey @mdneyazahmad, there are some outstanding comments in the PR, are you able to take a look? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Oct 13, 2022
@mdneyazahmad
Copy link
Contributor

Sorry for the delay. I will update it sometime today. Thanks!

@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2022
@MitchExpensify
Copy link
Contributor

Any update @mdneyazahmad ?

@melvin-bot melvin-bot bot removed the Overdue label Oct 25, 2022
@mdneyazahmad
Copy link
Contributor

@MitchExpensify PR is deployed on staging #10790 (comment)

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 26, 2022
@melvin-bot melvin-bot bot changed the title [$250] The ‘Multiple users are typing’ vertical text alignment is incorrect [HOLD for payment 2022-11-02] [$250] The ‘Multiple users are typing’ vertical text alignment is incorrect Oct 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 26, 2022

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

@MitchExpensify
Copy link
Contributor

Made a note to pay on 2022-11-02

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Nov 2, 2022
@Julesssss
Copy link
Contributor

Not overdue, awaiting payment

@melvin-bot melvin-bot bot removed the Overdue label Nov 2, 2022
@Julesssss Julesssss added Weekly KSv2 and removed Daily KSv2 labels Nov 2, 2022
@Julesssss
Copy link
Contributor

Julesssss commented Nov 2, 2022

Oh cool, it is the second of November today. Is that why the label changed priority? 🤯

@MitchExpensify
Copy link
Contributor

I had to repost the job and have sent you both offers @parasharrajat & @mdneyazahmad. Will pay and close this once accepted! Thank you

@mdneyazahmad
Copy link
Contributor

@MitchExpensify offer accepted. Thanks!

@MitchExpensify
Copy link
Contributor

Paid! Thanks all

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 Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants