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

Modify line height so text does not get cut off in LHN #4143

Merged
merged 2 commits into from
Jul 20, 2021

Conversation

yuwenmemon
Copy link
Contributor

@yuwenmemon yuwenmemon commented Jul 19, 2021

Pullerbearing (@Luke9389)
cc @roryabraham

Details

Fixes a line height so that text is not cut off in Android. h/t @parasharrajat for his comment here: #4074 (comment)

Fixed Issues

$ #4074
$ #4143

Tests/QA

  1. Create a chat with multiple participants
  2. Open up the LHN, make sure the commas and bottom ligatures of the text are not cut off.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-07-19 at 3 45 47 PM

Mobile Web

Screen Shot 2021-07-19 at 4 05 16 PM

Desktop

Screen Shot 2021-07-19 at 3 48 19 PMcd i

iOS

Screen Shot 2021-07-19 at 4 02 04 PM

Android

Screen Shot 2021-07-19 at 3 38 06 PM

@yuwenmemon yuwenmemon requested a review from a team July 19, 2021 23:05
@yuwenmemon yuwenmemon self-assigned this Jul 19, 2021
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team July 19, 2021 23:05
@OSBotify
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@shawnborton
Copy link
Contributor

I don't think we should make this change globally, so please hold on merging this.

@shawnborton
Copy link
Contributor

Okay so based on Rajat's comment, can you just add this lineHeight to the specific area that is being affected on Android? We should not be changing this globally as before Rory's change, the global line height (or at least what we tried to use as much as we could) was 20.

@shawnborton shawnborton self-requested a review July 19, 2021 23:13
@shawnborton
Copy link
Contributor

So basically just set 18 as the line height on optionDisplayName

@yuwenmemon
Copy link
Contributor Author

@shawnborton They're already at 18: https://github.com/Expensify/Expensify.cash/blob/0639c6771204ceb1c91926813fb2b3368c49a8b6/src/styles/styles.js#L803-L804

I can just set them to 20 to match the default? That also fixed the issue:
Screen Shot 2021-07-19 at 5 21 47 PM

@yuwenmemon yuwenmemon requested a review from a team as a code owner July 20, 2021 00:23
@MelvinBot MelvinBot requested review from Dal-Papa and removed request for a team July 20, 2021 00:24
@yuwenmemon yuwenmemon removed the request for review from Dal-Papa July 20, 2021 00:26
@shawnborton
Copy link
Contributor

Ah got it. Okay, I am cool with making these 20 line height so long as the line below is at 16 line height. Then, assuming there is a 4px gap between them, we get a total height of 40px which matches the avatar to the left.

@yuwenmemon
Copy link
Contributor Author

Ah got it. Okay, I am cool with making these 20 line height so long as the line below is at 16 line height.

Cool! That they are:

App/src/styles/styles.js

Lines 822 to 828 in f8c358e

optionAlternateText: {
color: themeColors.textSupporting,
fontFamily: fontFamily.GTA,
fontSize: variables.fontSizeLabel,
height: 16,
lineHeight: 16,
},

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@parasharrajat
Copy link
Member

Please test it on #focus mode. The issue is mainly visible in that case.

@yuwenmemon
Copy link
Contributor Author

@parasharrajat I assume you're referring to #4137?

Looks good too!
Screen Shot 2021-07-20 at 10 49 51 AM

@parasharrajat
Copy link
Member

Nice. There is one more place to look at the Report header. It is also affected by the same issue.

@yuwenmemon
Copy link
Contributor Author

@parasharrajat I'm sorry, but can you please point to an issue or something displaying what you're talking about?

@parasharrajat
Copy link
Member

Here you go
image

@yuwenmemon
Copy link
Contributor Author

yuwenmemon commented Jul 20, 2021

I think we're good there, here's a header with some letters with bottom-y parts on my branch, nothing is cut off?
Screen Shot 2021-07-20 at 10 58 26 AM

@yuwenmemon
Copy link
Contributor Author

Gonna merge this since Luke is OOO

@yuwenmemon yuwenmemon merged commit 029ed17 into main Jul 20, 2021
@yuwenmemon yuwenmemon deleted the yuwen-lineHeight branch July 20, 2021 17:59
github-actions bot pushed a commit that referenced this pull request Jul 20, 2021
Modify line height so text does not get cut off in LHN

(cherry picked from commit 029ed17)
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-3🚀

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

@isagoico
Copy link

Tested this on:

  • Web
  • Android
  • iOS
  • Desktop
    And all were a pass! 🎉

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants