[HOLD for regression tests 2023-01-25] Font style inconsistency in some areas of the app

[kavimuru]

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. Open NewDot
2. Open Preferences page, User details page and create new chat screen

Expected Result:

Small label style font should be consistent everywhere in the app. Ideally we want this to all be at our label size (13px) in our text supporting color using our regular weight font

Actual Result:

There is a inconsistency in the label style

Workaround:

unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.52-4
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1672827605509459

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e365d5cd722c09e2
• Upwork Job ID: 1612961027488239616
• Last Price Increase: 2023-01-10

[melvin-bot]

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01e365d5cd722c09e2

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @rushatgabhane (Internal)

[luacmartins]

PR ready for review - #14217

[rushatgabhane]

fyi, i didn't review the pr. it was done internally

[melvin-bot]

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

• Fix label fonts #14217

If no regressions arise, payment will be issued on 2023-01-25. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[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:

• [@luacmartins] The PR that introduced the bug has been identified. Link to the PR: N/A we have new fonts, so the previous ones were correct until now
• [@luacmartins] 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: N/A we have new fonts, so the previous ones were correct until now
• [@luacmartins] 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: N/A we have new fonts, so the previous ones were correct until now
• [@miljakljajic] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[miljakljajic]

No one to be paid (Reported by Shawn, Worked on by Carlos).

@shawnborton, would you say this Edit Information in Profile section would be the best place to add a test to check for font size? Given that from the slack conversation, the other sections will soon be refactored?

[shawnborton]

Hmm well I suppose the Edit Info in Profile page was already largely correct before this PR, we ended up changing the label font size in other places like Preferences, Payments, as well as the user selector in the Send/Request flows.

[miljakljajic]

Would it make more sense then to suggest we add a test that checks the font size across all those flows to confirm we're using the same one?

[shawnborton]

I think that could work, yeah.

To be honest though, this wasn't really a bug. This was just a matter of the various phases of NewDot development happening at different times. For example, the user selector in the Send/Request flow was created a long long time ago it followed a mockup I made that used small letters in all caps and bold. Later on, we started a font in regular letters that wasn't all caps for these kinds of labels. So I would say that everything was implemented as according to the mockups, but over time, we (I) saw some inconsistency in how we were treating those moments and wanted to do something to make everything look consistent.

[miljakljajic]

Maybe it isn't really something we'd create a new test for, in that case. I will get some feedback from the rest of the team. Thank you!

[shawnborton]

Not overdue

[miljakljajic]

bumped on the thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1674491805845709?thread_ts=1672827605.509459&cid=C01GTK53T8Q

[luacmartins]

Waiting on regression period!

[miljakljajic]

We've agreed we don't need a regression test for this case here.

Closing - thank you all!

[fedirjh]

@luacmartins , @shawnborton there is a regression with the associated PR 0479c58.
Inside BasePaymentsPage.js , the listHeaderComponent is missing the margin bottom , this was missed during fixing the font style .
The old formLabel style has marginBottom: 8 , However the textLabelSupporting doesn't have marginBottom set , so we should apply styles.mb2 to fix this regression .

--- a/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
+++ b/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
@@ -324,7 +324,7 @@ class BasePaymentsPage extends React.Component {
                     </>
                 )}
                 <Text
-                    style={[styles.ph5, styles.mt6, styles.textLabelSupporting]}
+                    style={[styles.ph5, styles.mt6, styles.textLabelSupporting, styles.mb2]}
                 >
                     {this.props.translate('paymentsPage.paymentMethodsTitle')}
                 </Text>

Before PR	After PR	After Fix

[shawnborton]

Hmm, what does it look like with mb1 added to it? And to clarify, this is the hover state you are showing right?

[fedirjh]

Hmm, what does it look like with mb1 added to it?

mb0	mb1	mb2

And to clarify, this is the hover state you are showing right?

@shawnborton yes that's hover state

[shawnborton]

Got it. It would be my preference to do .mb1 instead of .mb2. @luacmartins let me know what you think.

[luacmartins]

.mb1 looks good to me too!

[fedirjh]

@luacmartins, @shawnborton should I create required PR ?

[luacmartins]

@fedirjh do we have a bug report for the issue? If not, can we create one? You can tag me in Slack.

[fedirjh]

@luacmartins I have reported the bug in Slack and tagged you there .