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

Add new header font #13714

Merged
merged 44 commits into from
Jan 6, 2023
Merged

Add new header font #13714

merged 44 commits into from
Jan 6, 2023

Conversation

luacmartins
Copy link
Contributor

@luacmartins luacmartins commented Dec 20, 2022

Details

Updates our header font to New Kansas

Fixed Issues

$ #13686

Tests

Verify that the following places are using the new header font

  1. The Chatstext in the top left of the LHN
  2. The amount in + > Send money
  3. The card headers in the Workspace subpages, e.g. Workspace > Issue cards
  4. The display name in Settings, Chat header > User details and Workspace settings page
  5. The Not found page. Type an address that doesn't exist in the browser, e.g. /test. Can only be tested on Web and mWeb.
  6. The success page in Security > Change password. You'll have to change your password to see this page.
  7. The success page in Concierge chat > Request a call. Enter your details in the modal and request a call.
  8. The success page in Payments > Add bank account. Connect with Plaid, selecting Citi and the sandbox credentials user_good, pass_good.

C+ and QA can skip the following steps

  1. The Activate wallet success page. Follow this SO to test the flow and get to the success page.
  2. On another account, run ./clitools.sh validator:wallet and then send money to your test account + > Send money and pay with Expensify Wallet.
  3. On your test account, navigate to Payments > Transfer balance. Verify that the success page has the new font.
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as test steps.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

BNP

web

Workspace cards
web

Display names
web-details
web-profile
web-workspace

LHN header
web

Not found page
web

Activate wallet
web

Add bank account
web

Password confirm
web

Request call
web

Transfer balance
web

Room names
web

Protected PDF
web-pdf

Drag and drop modal
web-drag

Terms Step
web

Mobile Web - Chrome

BNP
chrome

Workspace cards
chrome

Display names
chrome-details
chrome-profile
chrome-workspace

LHN header
chrome

Not found page
chrome

Add bank account
chrome

Password confirm
chrome

Request call
chrome

Transfer balance
chrome

Room names
chrome

Mobile Web - Safari

BNP
safari

Workspace cards
safari

Display names
safari-details
safari-profile
safari-workspace

LHN header
safari

Not found page
safari

Add bank account
safari

Password confirm
safari

Request call
safari

Transfer balance
safari

Room names
safari

Desktop

BNP
desktop

Workspace cards
desktop

Display names
desktop-details
desktop-profile
desktop-workspace

LHN header
desktop

Add bank account
desktop

Password confirm
desktop

Request call
desktop

Transfer balance
desktop

Room names
desktop

Protected PDF
desktop-pdf

Drag and drop modal
desktop-drag

iOS

BNP
ios

Workspace cards
ios

Display names
ios-details
ios-profile
ios-workspace

LHN header
ios

Add bank account
ios

Password confirm
ios

Request call
ios

Transfer balance
ios

Room names
ios

Android

BNP
android

Workspace cards
android

Display names
android-details
android-profile
android-workspace

LHN header
android

Add bank account
android

Password confirm
android

Request call
android

Transfer balance
android

Room names
android

@luacmartins luacmartins self-assigned this Dec 20, 2022
@luacmartins
Copy link
Contributor Author

This is working well for me locally. Ready for review!

@luacmartins luacmartins marked this pull request as ready for review December 20, 2022 21:27
@luacmartins luacmartins requested a review from a team as a code owner December 20, 2022 21:27
@luacmartins
Copy link
Contributor Author

@shawnborton adding you as a reviewer!

@melvin-bot melvin-bot bot requested review from MariaHCD and parasharrajat and removed request for a team December 20, 2022 21:28
@melvin-bot
Copy link

melvin-bot bot commented Dec 20, 2022

@parasharrajat @MariaHCD One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@luacmartins luacmartins dismissed stale reviews from shawnborton, MariaHCD, and Luke9389 via c750c71 January 6, 2023 17:40
@luacmartins
Copy link
Contributor Author

We are looking good. Just missing the completed checklist and another approval.

@parasharrajat
Copy link
Member

parasharrajat commented Jan 6, 2023

Screenshots

🔲 iOS / native

screen-2023-01-06_23.39.24.mp4

🔲 iOS / Safari

screen-2023-01-06_23.44.25.mp4

🔲 MacOS / Desktop

screen-2023-01-06_23.38.08.mp4

🔲 MacOS / Chrome

screen-2023-01-06_23.19.29.mp4

🔲 Android / Chrome

Not able to test due to Sign in issues.

🔲 Android / native

screen-2023-01-06_23.23.43.mp4

@parasharrajat
Copy link
Member

Although, I can't test it on mweb because on Sign issues but I have marked it on the checklist for checks to pass. I also believe the changes to work fine on mweb due to only font changes and they work on web.

@luacmartins
Copy link
Contributor Author

@parasharrajat @Luke9389 would you mind re-approving this PR so we can get it merged?

@parasharrajat
Copy link
Member

Yeah, just 5 mins. Adding a few videos.

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

🎀 👀 🎀 C+ reviewed

@parasharrajat
Copy link
Member

Approved, Please assign me to the issue as well. Thanks.

@luacmartins
Copy link
Contributor Author

@parasharrajat thanks and I assigned you to the issue. We are just missing one internal approval now. @Luke9389 would you mind reviewing again since the other reviewers are out for the weekend already?

@Luke9389 Luke9389 merged commit 42f9f24 into main Jan 6, 2023
@Luke9389 Luke9389 deleted the cmartins-updateHeaderFonts branch January 6, 2023 19:54
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
TTI 821.534 ms → 713.033 ms (-108.501 ms, -13.2%) 🟢
Show details
Name Duration
TTI Baseline
Mean: 821.534 ms
Stdev: 39.633 ms (4.8%)
Runs: 734.7100520003587 759.0707590002567 770.6824880000204 771.3044140003622 779.7001109998673 791.3754489999264 792.9285469995812 793.2835510000587 795.3035800000653 807.0193739999086 807.7054479997605 809.9583010002971 812.060734000057 812.4998580003157 815.3107409998775 819.4179809996858 821.8006969997659 822.1729669999331 822.5268219998106 825.9890900002792 826.0739639997482 829.1203619996086 844.5373980002478 851.0088980002329 853.4087889995426 862.1994810001925 876.1195620000362 880.8051039995626 883.1228900002316 887.3929329998791 908.9429479995742

Current
Mean: 713.033 ms
Stdev: 43.656 ms (6.1%)
Runs: 654.9358860002831 657.9804680002853 660.6551839997992 668.5699909999967 670.4019259996712 671.9299440002069 671.9600630002096 678.0292920004576 678.949585000053 682.3154560001567 683.4906679997221 685.4945750003681 689.362191000022 691.0166189996526 694.6413799999282 703.1025050003082 704.5344390003011 708.856069999747 709.7326269997284 714.9747550003231 726.3706649998203 730.2848819997162 736.7838359996676 737.1598359998316 743.8926879996434 754.5173650002107 763.5956960003823 767.6691870000213 775.5739179998636 777.7773380000144 790.203846000135 832.2950689997524

Meaningless Changes To Duration

Show entries
Name Duration
runJsBundle 200.781 ms → 203.563 ms (+2.781 ms, +1.4%)
regularAppStart 0.015 ms → 0.015 ms (-0.001 ms, -3.8%)
nativeLaunch 10.097 ms → 9.813 ms (-0.284 ms, -2.8%)
Show details
Name Duration
runJsBundle Baseline
Mean: 200.781 ms
Stdev: 26.379 ms (13.1%)
Runs: 161 163 165 165 167 168 170 181 181 182 189 189 191 192 194 200 201 205 208 209 210 211 214 215 223 225 229 234 240 241 248 254

Current
Mean: 203.563 ms
Stdev: 30.005 ms (14.7%)
Runs: 159 163 164 171 172 173 176 176 179 180 183 190 190 192 192 199 202 204 207 209 211 215 216 223 224 227 235 248 253 255 260 266
regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (8.5%)
Runs: 0.01281800027936697 0.0134680001065135 0.013712000101804733 0.013794000260531902 0.014078999869525433 0.014159999787807465 0.014159999787807465 0.014281999319791794 0.014281999319791794 0.014282000251114368 0.0143630001693964 0.014527000486850739 0.014607000164687634 0.014933000318706036 0.014933000318706036 0.01501499954611063 0.015015000477433205 0.015056000091135502 0.015096000395715237 0.015177999623119831 0.015300000086426735 0.015381000004708767 0.01570700015872717 0.015992000699043274 0.01607200037688017 0.016195000149309635 0.016316000372171402 0.016764000058174133 0.017211999744176865 0.01827000081539154 0.01855399925261736

Current
Mean: 0.015 ms
Stdev: 0.001 ms (4.9%)
Runs: 0.013345999643206596 0.01355000026524067 0.013630999252200127 0.013631000183522701 0.013631000183522701 0.013753999955952168 0.013875000178813934 0.013875000178813934 0.013915999792516232 0.014282000251114368 0.014403999783098698 0.014566999860107899 0.014607999473810196 0.01464799977838993 0.014689999632537365 0.01469000056385994 0.014729000627994537 0.0147299999371171 0.014770999550819397 0.014891999773681164 0.014891999773681164 0.0148930000141263 0.0148930000141263 0.014932999387383461 0.01501499954611063 0.01521800085902214 0.015258999541401863 0.01574699953198433 0.015868999995291233 0.016315999440848827
nativeLaunch Baseline
Mean: 10.097 ms
Stdev: 1.802 ms (17.9%)
Runs: 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 11 11 11 11 11 12 12 13 14 14 14

Current
Mean: 9.813 ms
Stdev: 1.424 ms (14.5%)
Runs: 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 10 11 11 11 12 12 12 13 13

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Jan 6, 2023
@luacmartins
Copy link
Contributor Author

Not sure why this was marked as a blocker. The TTI was actually reduced by 13%!

@luacmartins luacmartins removed the DeployBlockerCash This issue or pull request should block deployment label Jan 6, 2023
@OSBotify
Copy link
Contributor

OSBotify commented Jan 6, 2023

🚀 Deployed to staging by @Luke9389 in version: 1.2.50-2 🚀

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

@roryabraham
Copy link
Contributor

Not sure why this was marked as a blocker. The TTI was actually reduced by 13%!

Made a follow-up issue to investigate this: #14123

@OSBotify
Copy link
Contributor

OSBotify commented Jan 9, 2023

🚀 Deployed to production by @Julesssss in version: 1.2.50-14 🚀

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Changes in this file resulted in a regression here. textHeadline had a line height different than the font size textXXXLarge. This caused the values to be cropped in android.

Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from #35435.
ExpensifyNewKansas has misaligned vertical metrics on OS/2, and now has been fixed. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants