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

Typing indicator ellipses #5933

Merged
merged 10 commits into from
Nov 2, 2021

Conversation

akshayasalvi
Copy link
Contributor

@akshayasalvi akshayasalvi commented Oct 18, 2021

Details

  • Added ellipses for long emails in typing indicator

Fixed Issues

$ #5623

Tests

  1. Tested email long email and display name
  2. Tested for small email
  3. Tested with multiple users

QA Steps

  1. Go to chat with any account A (or with a long email)
  2. Go to chat with account B
  3. Send chats from Account A to Account B, the typing indicator should show the trimmed username along with the is typing word visible
  4. Test the same with small email, it shouldn't trim the email
  5. For two or more users it should show Multiple users typing...

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web-long-email-without-ellipses

web-long-email-with-ellipses

web-normal-email

Mobile Web

mweb-email-ellipses

Desktop

desktop-email

iOS

ios-email-ellipses

Android

android-email-ellipses

@@ -160,7 +160,7 @@ export default {
newMsg: ({count}) => `${count} new message${count > 1 ? 's' : ''}`,
},
reportTypingIndicator: {
isTyping: 'is typing...',
isTyping: 'is typing',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep this as it is??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did add ... back to typing....

image

I am not sure if two ... look fine. But upto you guys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think it's fine for now since is typing... is pretty standard if there's no animation for a user typing.

Comment on lines 2188 to 2197
function getTypingIndicatorTextStyle(isSmallScreenWidth) {
return isSmallScreenWidth
? {
maxWidth: 200,
}
: {
maxWidth: 500,
};
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This is supposed to be fluid. Take max available space with hiding is typing...

<Text
style={[
styles.chatItemComposeSecondaryRowSubText,
styles.chatItemComposeSecondaryRowOffset,
getTypingIndicatorTextStyle(this.props.isSmallScreenWidth),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will not need it. See comment below...

<Text
style={[
styles.chatItemComposeSecondaryRowSubText,
styles.chatItemComposeSecondaryRowOffset,
styles.ml1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should take the natural gap of 1 char.

@akshayasalvi
Copy link
Contributor Author

@parasharrajat PR updated.

@@ -108,6 +100,7 @@ ReportTypingIndicator.displayName = 'ReportTypingIndicator';

export default compose(
withLocalize,
withWindowDimensions,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like unsued now.

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshayasalvi Did you test it on all platforms after the recent changes? If so, please update the screenshots.

I see that It does not work well on Android.
Screenshot 2021-10-20 04:38:25
The name should not trim until you have consumed all the available space. In the screenshot, there is plenty of space after is typing...
So the Name will be trimmed when is Typing... is close to the right edge where the composer ends.

@akshayasalvi
Copy link
Contributor Author

Yes, you're right. I tried and it does break in Android. I've tried to fix it but it then increases the whitespace between the email and is Typing. Will check.

@parasharrajat
Copy link
Member

Any update @akshayasalvi ?

@akshayasalvi
Copy link
Contributor Author

akshayasalvi commented Oct 25, 2021

Any update @akshayasalvi ?

Tried but haven't figured a solution that ensures fluid for all cases. Still checking.

@akshayasalvi
Copy link
Contributor Author

@parasharrajat @NikkiWines I've made changes and it works fine on Android but I am not sure it doesn't work fine on the Android emulator.

Attached is the screenshot from my android phone.

android-screenshot

@parasharrajat
Copy link
Member

I will check it later.

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor comments but it's looking great

<View style={[styles.chatItemComposeSecondaryRow, styles.flexRow]}>
<View style={[styles.chatItemComposeSecondaryRowOffset, styles.flexShrink1]}>
<Text
style={[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be a single line instead

style={[styles.chatItemComposeSecondaryRowSubText]}

</Text>
</View>
<View style={[styles.flexShrink0]}>
<Text
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also be a single line:

 <Text style={[styles.chatItemComposeSecondaryRowSubText]}>

parasharrajat
parasharrajat previously approved these changes Oct 29, 2021
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tests well. Please refer to Nikki's comments and we are good here. Thanks for the changes.

@akshayasalvi
Copy link
Contributor Author

@parasharrajat @NikkiWines PR updated

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good!!

@NikkiWines
Copy link
Contributor

@parasharrajat not sure if you wanted to give this another review or not.

If you haven't by EOD, I'll merge 🚀

@NikkiWines NikkiWines merged commit 6299f48 into Expensify:main Nov 2, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Nov 2, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Nov 3, 2021

🚀 Deployed to staging by @NikkiWines in version: 1.1.12-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@nazam100
Copy link

nazam100 commented Nov 3, 2021

Accessibility Issue : Incorrect focus order.
WCAG Failure : 2.4.3.
Expected result : The focus should reach to the chat box after navigating from + button(new chat). .
Actual result : The Focus is moved to menu 'Search tabs menu button subMenu'

Attachments :

Focus.order.fails.mp4

@nazam100
Copy link

nazam100 commented Nov 3, 2021

Accessibility Issue : Incorrect Focus order through messages is reversed bottom-to-top.
WCAG Failure : 2.4.3.
Expected result : The focus should follow the standard reading order of top-down, left-to-right.
Actual result : All of the elements in the chat window are focused in reverse order (bottom-up, left-to-right), making keyboard navigation very confusing. The same occurs with the reading order and scroll direction when the container has many messages (down moves up, up moves down).

Attachments :

Focus.order.fails.from.top.to.bottom.mp4

@nazam100
Copy link

nazam100 commented Nov 3, 2021

Accessibility Issue : "+" (plus), 'Emoji', 'Send' Buttons has wrong button role, state not announced. Similar to the bug #5677.
WCAG Failure : 4.1.2
Expected result : The buttons are not a regular buttons. It functions as a '+'(plus), Emoji', 'Send' Buttons and should announce as such with a collapsed/expanded state. i.e. '+'Button, Emoji' button, 'Send' Button.
Actual result :No '+' (plus). Emoji' button, 'Send' button role or state is announced. Only a Clickable role are announced without any further context.

Attachments :

name.role.vale.+.button.mp4

@OSBotify
Copy link
Contributor

OSBotify commented Nov 4, 2021

🚀 Deployed to production by @yuwenmemon in version: 1.1.13-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants