Skip to content

Jon/wallet-details-header#5450

Merged
Jon-edge merged 2 commits intodevelopfrom
jon/wallet-details-header
Feb 20, 2025
Merged

Jon/wallet-details-header#5450
Jon-edge merged 2 commits intodevelopfrom
jon/wallet-details-header

Conversation

@Jon-edge
Copy link
Collaborator

@Jon-edge Jon-edge commented Feb 11, 2025

For the TransactionList, keep the same title so the user knows which wallet the transactions are for.

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

component={WalletDetails}
options={{
headerTitle: () => <ParamHeaderTitle<'walletDetails'> fromParams={params => params.walletName} />
headerTitle: () => <ParamHeaderTitle<'walletDetails'> fromParams={params => params.sceneTitle} />
Copy link
Contributor

@swansontec swansontec Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the sceneTitle param, and put the string formatting in fromParams:

const account = useSelector(state => state.core.account)
const wallet = account.currencyWallets[walletId]
return sprintf(lstrings.create_wallet_account_metadata_name, wallet?.currencyInfo.displayName)

Or, better yet, export a WalletDetailsTitle component from WalletDetailsScene.tsx:

export const WalletDetailsTitle = () => {
  const route = useRoute<RouteProp<'walletDetails'>>()
  const account = useSelector(state => state.core.account)
  const wallet = account.currencyWallets[walletId]
  const title = sprintf(lstrings.create_wallet_account_metadata_name, wallet?.currencyInfo.displayName)
  return <HeaderTitle title={title} />
}

Then in Main.tsx, it can just be:

options={{ headerTitle: WalletDetailsTitle })

Oh, one more thing: The lstrings.create_wallet_account_metadata_name should be called lstrings.network_title_1s or something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the string name, yes, it should be that, but this is an old string. Since it's old and has translations already, we can't change the key or we reset the translations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense. We really need to figure out how to rename strings in Crowdin.

@Jon-edge Jon-edge force-pushed the jon/wallet-details-header branch from 031ac22 to 665da8e Compare February 18, 2025 22:21
export interface WalletDetailsParams {
walletId: string
sceneTitle: string
walletName: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So waletName isn't actually used anywhere. Please delete it, so the params are literally just walletId and tokenId...

It turns out that the countryCode route prop isn't really used either. Or, more specifically, there is only one place that actually provides it to WalletListScene, and that just does:

 const { countryCode } = await getFirstOpenInfo()
 navigation.navigate('walletDetails', { walletId, tokenId, countryCode })

The WalletDetailsScene doesn't use countryCode either. The only thing it does is pass countryCode to InfoCardCarousel and BuyCrypto, both of which will be buggy (showing wrong cards or wrong strings) if countryCode is undefined.

This means we should just refactor InfoCardCarousel and BuyCrypto to get their own country code:

const [ firstOpenInfo ] = useAsyncValue(async () => await getFirstOpenInfo())
const { countryCode } =  firstOpenInfo ?? {}

And then we can delete it the WalletDetailsScene courntryCode param.

Copy link
Collaborator Author

@Jon-edge Jon-edge Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely remember there was some reason we passed countryCode around instead of calling for it in the component but maybe it's no longer relevant...

Comment on lines +255 to +260
export const TransactionListTitle = () => {
const route = useRoute<RouteProp<'walletDetails'>>()
const account = useSelector(state => state.core.account)
const wallet = account.currencyWallets[route.params.walletId]
const title = wallet == null ? '' : getWalletName(wallet)
return <HeaderTitle title={title} />
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. But now that this is here, the walletName and countryCode route props are unused. For the the TransactionListScene, the route props should just be walletId, tokenId, and searchText.

@Jon-edge Jon-edge force-pushed the jon/wallet-details-header branch from 665da8e to 5cf0c8c Compare February 20, 2025 01:26
For the TransactionList, keep the same title so the user knows which wallet the transactions are for.
@Jon-edge Jon-edge force-pushed the jon/wallet-details-header branch 2 times, most recently from 7356a87 to d4bfd7a Compare February 20, 2025 20:06
It was possible that multiple callers could trigger multiple disk reads if `getFirstOpenInfo` got called concurrently before the initial disk read completes.
@Jon-edge Jon-edge force-pushed the jon/wallet-details-header branch from d4bfd7a to fb946f9 Compare February 20, 2025 20:08
@Jon-edge Jon-edge merged commit 646f721 into develop Feb 20, 2025
2 checks passed
@Jon-edge Jon-edge deleted the jon/wallet-details-header branch February 20, 2025 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants