-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Core Branding] Font Fixes (Web / Mobile) #13415
Conversation
Added screenshots for mobile web on emulators |
Nice, much improved! |
Waiting for android build for final screenshot, so I'm gonna open this for review now |
@mananjadhav @ctkochan22 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] |
It looks like mono styles don't work on android (on main as well). I posted screenshots |
I'm not thinking, this PR is for web styles. Do we want to allow styled mono on android/ios? Or do nothing and leave it as is for now |
We should definitely plan to fix mono - can you share screenshots of what that looks like? Also while you are at it, could you confirm that the new font is working on the native Android app? In terms of fixing mono though, we could totally do it in a separate PR, or bundle it into this one if you'd like to - your call! |
https://expensify.slack.com/archives/C01GTK53T8Q/p1670546775495159 sike ...looks like I broke it 😂. Gonna throw this back in WIP to fix |
Oh wow, but it looks like none of the regular Neue fonts are working correctly on Android? |
Would someone mind building this branch on android and sending a screenshot? |
Screenshots look great, nice and crispy - nice work Georgia! |
@mananjadhav we don't currently have regression test steps for fonts, but will be adding them as core branding changes are finalized. |
Okay thanks for the update. I’ve tested them the composer with all font styles. I’ve also completed the checklist. @ctkochan22 All yours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so I think we want to make some changes to this PR. Namely, the .woff
and .woff2
assets are being bundled in the iOS and Android apps, despite the fact that they are not used on that platform. This is something that I solved in this PR, and we can borrow part of that code for this PR
To fix this, I think we'll want to:
-
establish a directory structure like this:
assets/fonts/ ----------------|native // all the .otf fonts for the mobile app ----------------|web // all the .woff2 and .woff fonts for the web app (plus .otf for NewKansas, for now)
-
Make this change in react-native.config.js to use
assets/fonts/native
for native font assets. -
Update this line to copy web fonts from
assets/fonts/web
instead ofassets/fonts
-
Make sure we update this line to also look for the ExpensifyNewKansas
.otf
files on web.
New Kansas isn't actually ready to go yet, we haven't finalized the EULA - so that being said, it should have never been added to begin with. So if anything, I say we just rip out the NK files from this PR and then we follow up as a separate PR/issue to add NK when we get the green light. |
Ok, so knowing that we can just:
|
We should definitely not include Fabriga, that is not a brand font. Let's just get rid of the NK files for now and keep all of the font styles as they are, where are just using Expensify Neue for headline moments. |
Or yeah, like you said, just do nothing with the NK files that are there. |
I just renamed the New Kansas file to match postscript in the android assets folder so when we do decide to use it, it won't break. Is it going to be replaced in the future anyways? |
It won't be replaced, it's the actual font (and font files, I think) that we'll be using. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thanks for making those changes 🙇
Made changes and added new screenshots! |
@mananjadhav Can you please retest and provide updated screenshots? |
Reviewer Checklist
Screenshots/VideosMobile Web - ChromePending Mobile Web - SafariPending @roryabraham @grgia Tested again but unable to run this on mWeb - Chrome and Safari both due to the Auth errors. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
@@ -7,9 +7,9 @@ const fontFamily = { | |||
EXP_NEUE: 'ExpensifyNeue-Regular', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for thinking of this so late... but I guess semantically, it makes more sense to just call this "Expensify Neue" or in this case, 'ExpensifyNeue'
as that's the name of the font family. "Regular" is simply a weight of the font, like Bold or Semi-Bold or Light, etc. No need to fix this immediately, but just wanted to jot my thoughts down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! could we bundle this with the new kansas changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm down with that! cc @luacmartins as I saw you picked up the NK changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI we created a separate issue for this change: #13772
🚀 Deployed to staging by @roryabraham in version: 1.2.42-0 🚀
|
🚀 Deployed to production by @yuwenmemon in version: 1.2.42-2 🚀
|
Details
Fixes android fonts, and updates font-family for crispier fonts. Also fixes italic mono console error on IOS. Note that with our new fonts, we do not support mono italics on mobile.
IOS error:
For future reference, this article was helpful.
Fixed Issues
$ #13413
Tests
ExpensifyNeue-Regular
Offline tests
QA Steps
*bold*, _italic_, *_italic and bold_*, ~strikethrough~, and regular chat
`mono` (ex: *`mono bold`*)
ExpensifyNeue-Regular
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android