-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[No QA] Fix Style on FullPageOfflineBlockingView #9269
Conversation
@shawnborton |
Hmm, what size do page headers use? That's the size we're using in Figma, which is 17 |
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
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.
Do you mind updating with some basic QA steps?
Also what conversation/issue is this referring to? Does this affect multiple platforms? |
@johnmlee101 I put the issue in the description. Here's the slack convo: https://expensify.slack.com/archives/C02HWMSMZEC/p1654010563728389 QA is a little tricky at the moment. I've been hardcoding the offline condition to @shawnborton Here's the Line 181 in 0e3bbe7
fontSize: variables.fontSizeh1, is defined here: Line 24 in 0e3bbe7
Again, totally fine with bumping it down to 17 if it looks too large in the screenshot. |
I'm not suggesting that we change the h1 style, but rather, just use the same style applied to the page header. Can you tell me more about that? Where do we store a font value as 17, and what do we call it? |
Ahh, OK. We call font size 17 fontSizeLarge. The titles we use for the pages (like "Connect Bank Account" in the screenshot) use the Line 256 in 0e3bbe7
OK, so I'll use that style so that they match. 👍 |
Perfect, thanks! |
Also, could you please replace |
Will wait for your final design check @shawnborton before finalizing my review 😄 |
src/styles/variables.js
Outdated
@@ -36,6 +36,7 @@ export default { | |||
iconSizeLarge: 24, | |||
iconSizeXLarge: 28, | |||
iconSizeExtraLarge: 40, | |||
iconSizeMondo: 60, |
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.
What does Mondo mean? I wonder if we should name this something more related to the usecase... like iconSizeFullPageOverlay
or something?
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.
How about iconSizeSuperDuperLarge?
I don't think we should tie it to the use case because it may be used in a different situation where we aren't doing a full page overlay.
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.
Updated to iconSizeSuperLarge
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.
Fair point. Another approach we can take is that if we assume a default icon is 20x20, this particular icon is 3x that size.
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 think the current name (iconSizeSuperLarge
) is fine.
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.
Cool, I don't feel strongly but at some point it would be nice to clean up these variable names.
Looks good to me! Just left one comment about the icon size variable. |
Anything else @johnmlee101? |
🚀 Deployed to production by @yuwenmemon in version: 1.1.73-2 🚀
|
Fixes
#9268
Hey Shawn, here are those style fixes.
Here's the current component (I'll update this as we do more fixes):