-
Notifications
You must be signed in to change notification settings - Fork 255
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
jon/scene-navigation-types #5320
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.
I have some suggestions, but I'm approving this anyhow because it's such a massive improvement. If you can get the suggestions working in, say, 1h of work, then great. Otherwise, let's get this merged regardless.
@@ -100,6 +104,8 @@ export type BuyTabParamList = {} & { | |||
rewardsCardWelcome: RewardsCardWelcomeParams | |||
} | |||
|
|||
export type SellTabParamList = {} & BuyTabParamList |
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.
If we are going to do this, we should do it for real, so:
type SellTabParamList = Omit<BuyTabParamList, 'pluginListBuy' | 'pluginViewBuy'> & {
pluginListSell: GuiPluginListParams | undefined
pluginViewSell: PluginViewParams
}
And delete the pluginListSell
and pluginViewSell
from BuyTabParamList
export function fakeRootSceneProps<Name extends keyof RootParamList>(name: Name, params: RootParamList[Name]): RootSceneProps<Name> { | ||
const navigation: RootSceneProps<Name>['navigation'] = fakeCompositeNavigation | ||
|
||
const route: RootSceneProps<Name>['route'] = { | ||
key: `${String(name)}-0`, | ||
name: name as Extract<Name, string>, | ||
params | ||
} | ||
|
||
const sceneProps: RootSceneProps<Name> = { | ||
navigation, | ||
route | ||
} | ||
|
||
return sceneProps | ||
} |
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.
We are copy-pasting a lot of lines of code here. We could simplify these guys by leveraging the existing fakeSceneProps
implementation, but hacking the types:
export function fakeRootSceneProps<Name extends keyof RootParamList>(name: Name, params: RootParamList[Name]): RootSceneProps<Name> {
return fakeSceneProps(name, params as any) as any
}
export function fakeDrawerSceneProps<Name extends keyof DrawerParamList>(name: Name, params: DrawerParamList[Name]): DrawerSceneProps<Name> {
return fakeSceneProps(name, params as any) as any
}
// ... and thus for the rest
// EdgeAppSceneProps<'pluginView'>['route'] | | ||
// BuyTabSceneProps<'pluginViewBuy'>['route'] | | ||
// SellTabSceneProps<'pluginViewSell'>['route'] | ||
// TODO: Break up these "GuiPlugin" scenes. | ||
route: any |
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.
Oh, this breaking these up is a good idea! You could do:
export function PluginViewBuy(props: BuyTabSceneProps<'pluginViewBuy'>) {
const {navigation, params} = props
return <GuiPluginViewScene navigation={navigation as NavigationBase} params={params} />
export function PluginViewSell(props: SellTabSceneProps<'pluginViewSell'>) {
const {navigation, params} = props
return <GuiPluginViewScene navigation={navigation as NavigationBase} params={params} />
export function PluginView(props: EdgeAppSceneProps<'pluginView'>) {
const {navigation, params} = props
return <GuiPluginViewScene navigation={navigation as NavigationBase} params={params} />
}
And then GuiPluginViewScene
can take {navigation: NavigationBase, params: PluginViewParams }
. We can solve the NavigationBase
thing in a future PR.
5539dde
to
a4c8b55
Compare
Made a spinoff task to address the comments: https://app.asana.com/0/1200972350208303/1208644065408258/f |
*Includes a small fix for an unreachable nullish operator operand in `SweepPrivateKeyCompletionScene`
a4c8b55
to
fdb87a6
Compare
*Includes a small fix for an unreachable nullish operator operand in
SweepPrivateKeyCompletionScene
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: