-
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
[TS migration] Migrate 'ExpensifyWordmark.js' component to TypeScript #31478
[TS migration] Migrate 'ExpensifyWordmark.js' component to TypeScript #31478
Conversation
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 😄
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeiOS: mWeb Safari |
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25084 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
src/styles/StyleUtils.ts
Outdated
@@ -462,7 +462,7 @@ function getBorderColorStyle(borderColor: string): ViewStyle { | |||
/** | |||
* Returns the width style for the wordmark logo on the sign in page | |||
*/ | |||
function getSignInWordmarkWidthStyle(environment: string, isSmallScreenWidth: boolean): ViewStyle { | |||
function getSignInWordmarkWidthStyle(environment: ValueOf<typeof CONST.ENVIRONMENT> | undefined, isSmallScreenWidth: boolean): ViewStyle { |
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.
Is there a shorthand way to set the param type here? Asking as I'm new to Typescript but familiar with Kotlin/Swift.
Also, is making this optional really necessary? It looks like the only usage of this function will reliably return an Environment... or is this due to a potential issue with EnvironmentContext
? Thanks!
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.
@Julesssss We can switch environment
and isSmallScreenWidth
params places and then mark environment
param as optional this way:
function getSignInWordmarkWidthStyle(isSmallScreenWidth: boolean, environment?: ValueOf<typeof CONST.ENVIRONMENT>): ViewStyle {
Yeah, since EnvironmentContext
can be null
, it's better to keep it optional.
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 thanks.
I'm not too bothered about the param ordering, but environment?:
seems a bit cleaner to me. But if this goes against typical formatting rules please ignore me
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.
@Julesssss Updated 🙂
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.
@fabioh8010 @Julesssss @VickyStash Shouldn't environment be required? I think we should throw an exception instead of returning an empty object in useEnvironment because EnrivonmentProvider is passed in root app. So that will allow us to remove possible null from getSignInWordmarkWidthStyle. I think there is no case when the environment is null.
What do you think about this approach?
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.
@ArekChr yeah if it is possible to enforce a non-optional environment, that sounds good to me. Happy to review a PR if you could raise one
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.
@ArekChr @fabioh8010
I think we can also try to use these default values for the EnvironmentContext instead of null.
const EnvironmentContext = createContext<EnvironmentContextValue>({
environment: CONST.ENVIRONMENT.PRODUCTION,
environmentURL: CONST.NEW_EXPENSIFY_URL,
});
It should be ok since these values are also used as default state values here and this way the environment will be non-optional value.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.1-13 🚀
|
Details
[TS migration] Migrate 'ExpensifyWordmark.js' component to TypeScript
Fixed Issues
$ #25084
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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)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.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