-
Notifications
You must be signed in to change notification settings - Fork 3k
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 Expensify Mono font and update code styling #54773
Conversation
Concierge reviewer checklist:
For more detailed instructions on completing this checklist, see How do I review a HelpDot PR as a Concierge Team member? cc @slafortune |
@shubham1206agra @slafortune 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] |
@@ -144,6 +144,21 @@ p { | |||
padding-bottom: 20px; | |||
} | |||
|
|||
code { | |||
font-family: 'Expensify Mono', 'Helvetica Neue', 'Helvetica', Arial, sans-serif; |
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 feel like the fallback fonts should be mono fonts and not helvetica, etc?
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.
Ah, yes I'll make that change.
@JoshIri360 The commits must have verified signatures. Please fix this. |
- Introduced 'Expensify Mono' font with regular and bold weights in _fonts.scss. - Updated _main.scss to apply 'Expensify Mono' font to code elements with custom styling. - Added new font files: ExpensifyMono-Regular.woff, ExpensifyMono-Regular.woff2, ExpensifyMono-Bold.woff, and ExpensifyMono-Bold.woff2.
d306829
to
d592c1b
Compare
@shubham1206agra I've fixed this now |
Reviewer Checklist
Screenshots/Videos |
@dubielzyk-expensify This PR is focused on font-related changes (adding Expensify Mono font and updating typography styles). The light/dark mode logo issue appears to be unrelated, but the logo used in my screenshot is different from the dark one shown here, so this may be fixed. |
Cool. Otherwise I think this looks good, but @Expensify/design for last gut check. |
Yup, this works for me - LGTM. |
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.
Code LGTM, thanks!
Actually. I'll just submit a bug fix for this 👍 |
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.0.82-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.0.82-0 🚀
|
@@ -131,7 +131,7 @@ details[open] > summary { | |||
|
|||
h1, | |||
summary { | |||
font-family: 'Expensify New Kansas', 'Helvetica Neue', 'Helvetica', Arial, sans-serif; | |||
font-family: 'Expensify Mono', 'SFMono-Regular', monospace, Arial, sans-serif; |
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.
@JoshIri360 why did we update the h1 styles to use mono?
Also, how is the summary
tag used across the site? What does it do exactly?
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.
Think this was answered in the other PR, but this is all fixed now :)
Looks like a mistake on my end, this shouldn’t be Mono font as it wasn’t in my proposal. It looks like it’s been fixed already in another PR, but let me know if I need to do anything else. I’ll make sure to double check moving forward, sorry about that. |
No all good. I spotted it in the review, but assumed it was testing when I should've raised it, so equally on my end 😄 No biggie 👍 |
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.82-12 🚀
|
Explanation of Change
Fixed Issues
$ #54227
PROPOSAL: #54227 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop