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

Android - LHN - Period is displayed in between the user names of group chat in unread messages #4074

Closed
kavimuru opened this issue Jul 15, 2021 · 10 comments
Assignees
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2 Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Jul 15, 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

Expected Result:

Comma is displayed in between the user names for both read and unread groups

Actual Result:

Period is displayed in between the user names for the unread groups

Workaround:

Unknown

Platform:

Where is this issue occurring?

Web
iOS
Android ✔️
Desktop App
Mobile Web

Version Number:
1.0.78-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

@kavimuru kavimuru added the DeployBlockerCash This issue or pull request should block deployment label Jul 15, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

@roryabraham I think this is a side-effect of replacing the Text with ExpensiText. I found that we have height:18 set on these styles and if we remove it will be fixed. Also if you look closely, you will found that in focused mode the Chat title is not centered with respect to Avatar.

@yuwenmemon yuwenmemon assigned roryabraham and unassigned yuwenmemon Jul 15, 2021
@roryabraham
Copy link
Contributor

Sorry I didn't get to this yesterday. I'll work on getting this resolved now.

@roryabraham
Copy link
Contributor

Confirmed that this doesn't seem to be happening on other platforms.

@roryabraham
Copy link
Contributor

Sorry, I had a bunch of issues with my Android emulators + not being able to update the hosts file to access the VM API. So I wasn't able to reproduce this. Lame, I know, but I have to give this back to you @yuwenmemon because I'll be OOO next week. I'm guessing it's just a simple issue with the default lineHeight here

@isagoico
Copy link

Issue reproducible during KI retests

@yuwenmemon
Copy link
Contributor

Hmmm... I'm actually not seeing this on the latest version of Master:
Screen Shot 2021-07-19 at 11 48 35 AM

What kind of Android device was it specifically?

@parasharrajat
Copy link
Member

So @yuwenmemon This issue started showing up when Rory moved the RN text to our Text component. Previously Line-height and height were set to 18px which had no issues. https://github.com/Expensify/Expensify.cash/blob/47494884adb24d23226edb0eb7d22d8240ecf979/src/styles/styles.js#L801-L806.

Now line height will be taken from Text component default which is 20 so issues are coming. We are in discussion on removing the line height from the Text component as it has caused many issues. But to fix this.
either we set the line height back to 18 or remove the height.

@isagoico
Copy link

Retested this after #4143 was CPd and it was a pass 🎉 Closing the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants