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.

Sign up for GitHub

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

Jump to bottom

Only provide a default lineHeight when the fontSize is default #3966

Merged
merged 2 commits into from
Jul 13, 2021
Merged

Only provide a default lineHeight when the fontSize is default #3966

merged 2 commits into from
Jul 13, 2021

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Jul 10, 2021 •
edited
Loading

Details

Our custom Text component provided a default line height. However, for larger fonts, that caused the text to appear cut off. To fix, we only provide a default line height when the font size is the default.

Fixed Issues

$ #3964

Tests / QA Steps

  1. Open the app
  2. Create a new bill split
  3. Verify that the currency/amount is fully visible.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

image

Desktop

image

iOS

image

Android

image

@roryabraham roryabraham self-assigned this Jul 10, 2021
@roryabraham roryabraham requested a review from a team as a code owner July 10, 2021 00:01
@MelvinBot MelvinBot requested review from deetergp and removed request for a team July 10, 2021 00:02
@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.

deetergp
deetergp previously requested changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

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

Considering the initial issue was logged on Android, it is espcially important that it be tested on Android. Disagree?

@roryabraham
Copy link
Contributor Author

Considering the initial issue was logged on Android, it is espcially important that it be tested on Android

Agree! I was having emulator issues before I ran out of time EOD on Friday when I created this PR, but I will work on getting the Android testing done here.

@roryabraham
Copy link
Contributor Author

Sorry about that @deetergp, G2G now 👍

@roryabraham roryabraham requested a review from deetergp July 12, 2021 18:27
@roryabraham
Copy link
Contributor Author

@deetergp Friendly bump because this resolves an hourly deploy blocker

@roryabraham roryabraham mentioned this pull request Jul 12, 2021
Closed
@thienlnam
Copy link
Contributor

Hmm looks like it still needs an approval from @deetergp (Since this is an hourly deploy blocker and Scott's comment was addressed going to see if I can unassign to merge)

@thienlnam thienlnam requested review from deetergp and removed request for deetergp July 13, 2021 00:00
@mountiny
Copy link
Contributor

@thienlnam You need to Dismiss his review

@mountiny mountiny dismissed deetergp’s stale review July 13, 2021 00:01

Hourly deploy blocker, need to merge

@thienlnam thienlnam merged commit d0c9578 into main Jul 13, 2021
@thienlnam thienlnam deleted the Rory-FixLineHeight branch July 13, 2021 00:02
@thienlnam
Copy link
Contributor

@mountiny Thanks!

github-actions bot pushed a commit that referenced this pull request Jul 13, 2021
@thienlnam
Merge pull request #3966 from Expensify/Rory-FixLineHeight
3d07b7a 
Only provide a default lineHeight when the fontSize is default

(cherry picked from commit d0c9578)
This was referenced Jul 13, 2021
Merged
Closed
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging in version: 1.0.77-2🚀

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

@OSBotify OSBotify mentioned this pull request Jul 14, 2021
Closed
95 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.77-5🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

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
Reviewers

@thienlnam thienlnam thienlnam approved these changes

@Dal-Papa Dal-Papa Dal-Papa approved these changes

@deetergp deetergp deetergp left review comments

Assignees

@roryabraham roryabraham

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@roryabraham @OSBotify @thienlnam @mountiny @deetergp @Dal-Papa