[HOLD for payment 2024-06-28] Default line height is 21 instead of 20

[shawnborton]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.4.81-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): @shawnborton @Expensify/design
Logs: N/A
Expensify/Expensify Issue URL: N/A
Issue reported by: @shawnborton
Slack conversation: Link

Action Performed:

View any default font size in the app

Expected Result:

The default line height should be 20

Actual Result:

The default line height is 21. This is caused by a commit introduced here by @j-piasecki

Workaround:

N/A

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @jliexpensify

[melvin-bot]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[shawnborton]

Also just noting that we do not want to accept external proposals for this, as @j-piasecki plans to create a fix for this.

[jliexpensify]

@j-piasecki hello, do you mind commenting on this issue so I can assign you? Cheers.

[j-piasecki]

@jliexpensify Ah, sorry for the delay, I didn't see the notification

[j-piasecki]

Update: The problem with emojis being cut off from the top looks to be somewhat related to the font being used - it's broken when using Expensify Neue and Arial but looks ok when using Segoe. I've found that on the new arch, the baseline is moved by this code fragment. After removing it, the emojis are displayed whole.

I'm a bit confused as the exact same code exists on the old architecture. I'll try to figure out what's causing the difference in behavior tomorrow.

[j-piasecki]

Update: opened up an issue upstream: facebook/react-native#44929, alongside PR fixing it: facebook/react-native#44932. I'll open a PR fixing it in the App assuming there's nothing wrong with that approach.

[shawnborton]

Amazing, thanks for doing that!

[j-piasecki]

Upstream PR got merged, so I opened a PR that adds a patch with the same changes to the App: #43902

[melvin-bot]

Triggered auto assignment to @mountiny, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.0-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Restore default line height to 20 #43902

If no regressions arise, payment will be issued on 2024-06-28. 🎊

For reference, here are some details about the assignees on this issue:

• @j-piasecki does not require payment (Contractor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@mountiny] The PR that introduced the bug has been identified. Link to the PR:
• [@mountiny] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@mountiny] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@j-piasecki] Determine if we should create a regression test for this bug.
• [@j-piasecki] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mananjadhav]

@jliexpensify Can you assign me this issue. I helped with the PR review on this one.

[jliexpensify]

Guessing this is the payment summary?

• C+: @mananjadhav $250 (ND payment)

Any checklist needed?

[mananjadhav]

That's all @jliexpensify. There's no checklist / regression test needed here. I am going to use the previous comment for the payment summary.

[JmillsExpensify]

$250 approved for @mananjadhav

[jliexpensify]

Great, thanks! Closing this out.