refactor(wallet-mobile): Receive funnel fixes and refactor #3092
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
stackchain
commented
Feb 26, 2024
•
stackchain
commented
- query fixes missing wallet.id
- collocated code
- renamed the screens to reflect the use cases
- bug fixes
- major bug on transfer (not resetting state properly)
- dropped unnecessary code
- refactored the hooks
stackchain
commented
Feb 26, 2024
| @@ -971,4 +971,4 @@ SPEC CHECKSUMS: | |||
|
|
|||
| PODFILE CHECKSUM: 4045625f1556b4d232075df8dbd22524a7010c3f | |||
|
|
|||
| COCOAPODS: 1.14.3 | |||
| COCOAPODS: 1.15.2 | |||
There was a problem hiding this comment.
all bump your Xcodes / cocoa pods
stackchain
commented
Feb 26, 2024
|
|
||
| const ACTION_PROPS = { | ||
| size: 24, | ||
| color: String(colors.actionColor), |
There was a problem hiding this comment.
dropped unnecessary String
stackchain
commented
Feb 26, 2024
| const wallet = useSelectedWallet() | ||
|
|
||
| const {isSingle, addressDerivation} = useAddressDerivationManager() | ||
| const {next: nextReceiveAddress} = useReceiveAddressesStatus(addressDerivation) |
There was a problem hiding this comment.
on ActionsBanner, there is no receive address selected, only the next that depends on wallet state, the context is only for the details, since we avoid params.
stackchain
force-pushed
the
refactor/receive-funnel
branch
from
1fc7ecd to
32e87a1
Compare
stackchain
commented
Feb 26, 2024
| const stakingKey = useStakingKey(wallet) | ||
| const swapManager = React.useMemo(() => { | ||
| const aggregatorTokenId = isMainnetNetworkId(wallet.networkId) ? milkTokenId.mainnet : milkTokenId.preprod |
stackchain
commented
Feb 26, 2024
| @@ -75,19 +75,15 @@ export const TxHistoryNavigator = () => { | |||
| const storage = useAsyncStorage() | |||
| const {theme} = useTheme() | |||
|
|
|||
| // receive | |||
| const receiveAddresses = useReceiveAddresses(wallet) | |||
stackchain
commented
Feb 26, 2024
| @@ -126,7 +122,7 @@ export const TxHistoryNavigator = () => { | |||
| const headerRightHistory = React.useCallback(() => <HeaderRightHistory />, []) | |||
|
|
|||
| return ( | |||
| <ReceiveProvider key={wallet.id} initialCurrentAddress={currentAddress}> | |||
| <ReceiveProvider key={wallet.id}> | |||
There was a problem hiding this comment.
initial state is mainly for testing / stories.
stackchain
commented
Feb 26, 2024
| <ReceiveProvider> | ||
| <AddressDetailCard address={mocks.address} title="Test Title" /> | ||
| </ReceiveProvider> | ||
| .addDecorator((story) => ( |
There was a problem hiding this comment.
moved the provider as decorator.
stackchain
commented
Feb 26, 2024
| import {useKeyHashes} from '../../../../yoroi-wallets/hooks' | ||
| import {useReceive} from '../ReceiveProvider' | ||
| import {ShareDetailsCard} from '../ShareDetailsCard/ShareDetailsCard' | ||
| import {ShareQRCodeCard} from '../ShareQRCodeCard/ShareQRCodeCard' | ||
|
|
||
| type ShareProps = { | ||
| address: string | ||
| type AddressDetailCardProps = { |
stackchain
commented
Feb 26, 2024
| if (selectedAddress === undefined) { | ||
| return | ||
| const index = Math.floor(offset / (itemsPerPage * screenWidth - minToSwitchPage)) | ||
| setScrollPosition(Math.max(index, 0)) |
There was a problem hiding this comment.
otherwise when swiping left at first page will cause no index be off while holding the first page, causing the indicator to be wrong until the page is released.
stackchain
commented
Feb 26, 2024
| title, | ||
| }, | ||
| ] as const | ||
|
|
||
| const screenWidth = useWindowDimensions().width |
stackchain
commented
Feb 26, 2024
| const totalPages = Math.ceil(cards.length / itemsPerPage) | ||
| const circleIndex = Array.from({length: totalPages}, (_, index) => index + 1) | ||
| const cardIndicators = Array.from({length: totalPages}, (_, index) => index) |
stackchain
commented
Feb 26, 2024
| } & ( | ||
| | { | ||
| cardType: 'QRCode' | ||
| } | ||
| | ({cardType: 'Details'} & AddressDetailsProps) | ||
| ) | ||
|
|
||
| export const AddressDetailCard = ({title}: ShareProps) => { | ||
| const [scrollPosition, setScrollPosition] = useState(0) | ||
| const [isSecondPage, setIsSecondPage] = useState(false) |
There was a problem hiding this comment.
dropped unnecessary state
stackchain
commented
Feb 26, 2024
| onLongPress={() => copy(item.address)} | ||
| isCopying={isCopying} | ||
| /> | ||
| ) | ||
| case 'Details': | ||
| if (Boolean(item.address) && Boolean(item.stakingHash) && Boolean(item.spendingHash) && Boolean(item.title)) { | ||
| setIsSecondPage(true) |
stackchain
commented
Feb 26, 2024
| <ShareDetailsCard address={item.address} stakingHash={item.stakingHash} spendingHash={item.spendingHash} /> | ||
| ) | ||
| default: | ||
| return null |
stackchain
commented
Feb 26, 2024
| @@ -116,21 +104,19 @@ export const AddressDetailCard = ({title}: ShareProps) => { | |||
|
|
|||
| <Spacer height={12} /> | |||
|
|
|||
| {isSecondPage && ( | |||
stackchain
commented
Feb 26, 2024
| @@ -156,8 +142,8 @@ const useStyles = () => { | |||
| }) | |||
|
|
|||
| const colors = { | |||
| indexActive: theme.color.primary[600], | |||
stackchain
commented
Feb 26, 2024
| @@ -8,26 +8,14 @@ import {Spacer, Text} from '../../../../components' | |||
| import {YoroiLogoIllustration} from '../../illustrations/YoroiLogo' | |||
|
|
|||
| type ShareProps = { | |||
| address?: string | |||
| title?: string | |||
There was a problem hiding this comment.
a lot of unused props
stackchain
commented
Feb 26, 2024
| } | ||
|
|
||
| type AddressDetailsProps = { | ||
| address: string |
stackchain
commented
Feb 26, 2024
| const {styles, colors} = useStyles() | ||
|
|
||
| return ( | ||
| <View style={[styles.touchableCard]}> |
stackchain
commented
Feb 26, 2024
| @@ -37,23 +25,23 @@ export const CaptureShareQRCodeCard = ({address}: ShareProps) => { | |||
|
|
|||
| <Spacer height={16} /> | |||
|
|
|||
| <YoroiLogoIllustration height={logoHeight} width={logoWidth} /> | |||
| <YoroiLogoIllustration height={37} width={35} /> | |||
There was a problem hiding this comment.
no need for constant, is not a magic number, the api is very declarative
stackchain
commented
Feb 26, 2024
| }: { | ||
| children: React.ReactNode | ||
| initialState?: Partial<ReceiveState> | ||
| initialCurrentAddress?: string |
There was a problem hiding this comment.
dropped part of the state already
stackchain
commented
Feb 26, 2024
| }) => { | ||
| const [state, dispatch] = React.useReducer(receiveReducer, { | ||
| ...defaultState, | ||
| ...initialState, | ||
| }) | ||
|
|
||
| const actions = React.useRef<ReceiveActions>({ | ||
| currentAddressChanged: (address: string) => dispatch({type: ReceiveActionType.CurrentAddressChanged, address}), | ||
| selectedAddressChanged: (address: string) => dispatch({type: ReceiveActionType.SelectedAddressChanged, address}), |
stackchain
commented
Feb 26, 2024
| } | ||
|
|
||
| const defaultState: ReceiveState = Object.freeze({ | ||
| selectedAddress: null, | ||
| defaultAddress: null, | ||
| selectedAddress: '', |
There was a problem hiding this comment.
receive address can't never be null, since is created on wallet basis
stackchain
commented
Feb 26, 2024
| stakingHash={mocks.stakinghash} | ||
| title="Test Title" | ||
| /> | ||
| <ShareDetailsCard address={mocks.address} spendingHash={mocks.spendinghash} stakingHash={mocks.stakinghash} /> |
stackchain
commented
Feb 26, 2024
| @@ -7,7 +8,9 @@ import {ShareQRCodeCard} from './ShareQRCodeCard' | |||
|
|
|||
| storiesOf('Receive ShareQRCodeCard', module) | |||
| .addDecorator((story) => <View style={styles.container}>{story()}</View>) | |||
| .add('default', () => <ShareQRCodeCard address={mocks.address} />) | |||
| .add('with content', () => ( | |||
| <ShareQRCodeCard content={mocks.address} onLongPress={action('onLongPress')} title="Title" /> | |||
There was a problem hiding this comment.
most components in common are actually useCases of just one screen, it needs a second refactor later
stackchain
commented
Feb 26, 2024
|
|
||
| const query = useQuery({ | ||
| suspense: true, | ||
| queryKey: [wallet.id, isShowingMultipleAddressInfoKey], |
stackchain
commented
Feb 26, 2024
|
|
||
| const parsed = parseSafe(storedStorage) | ||
| // old wallets wont have this key, so we default to true | ||
| return isBoolean(parsed) ? parsed : true |
There was a problem hiding this comment.
marking to display by default (empty storage), it affects only multiple funnel so needs to be configured, looks right needs confirmation cc: @SorinC6
stackchain
commented
Feb 26, 2024
| @@ -7,7 +7,7 @@ export const useNavigateTo = () => { | |||
| const navigation = useNavigation<TxHistoryRouteNavigation>() | |||
|
|
|||
| return useRef({ | |||
| receiceDetails: () => navigation.navigate('receive-single'), | |||
| receiveDetails: () => navigation.navigate('receive-single'), | |||
stackchain
commented
Feb 26, 2024
There was a problem hiding this comment.
also part of package candidate
stackchain
commented
Feb 26, 2024
| {used: [], unused: []} as Omit<ReceiveAddressesStatus, 'next'>, | ||
| ) | ||
| const multipleAddress = addressesStatus.unused[0] ?? addressesStatus.used[0] | ||
| const nextAddress = isSingle ? singleAddress : multipleAddress |
There was a problem hiding this comment.
next can only be calculated after the unused are discovered for multiple addresses, (first unused), if single always path index 0
stackchain
commented
Feb 26, 2024
| @@ -57,18 +38,16 @@ export const SingleAddressScreen = () => { | |||
| textStyles={{ | |||
| color: colors.buttonBackgroundBlue, | |||
| }} | |||
| onPress={() => navigate.specificAmount()} | |||
| disabled={currentAddress === null} | |||
| onPress={navigate.specificAmount} | |||
stackchain
commented
Feb 26, 2024
| address: string | ||
| } | ||
|
|
||
| export const MultipleAddressesScreen = () => { | ||
| export const ListMultipleAddressesScreen = () => { |
stackchain
commented
Feb 26, 2024
| }} | ||
| // date={mocks.usedAddressDate} // TODO don't have the date?? | ||
| // date={} // TODO define with project |
There was a problem hiding this comment.
needs checking, it should be dropped for the 1st iteration, it will require to scrap all txs. cc: @SorinC6
stackchain
commented
Feb 26, 2024
| ) | ||
|
|
||
| return ( | ||
| <SafeAreaView style={styles.root} edges={['left', 'right', 'bottom']}> | ||
| {mappedAddresses.length >= BIP32_HD_GAP_LIMIT && ( | ||
| {hasReachedGapLimit && ( |
There was a problem hiding this comment.
clean ups less overhead, declarative
stackchain
commented
Feb 26, 2024
| @@ -159,7 +160,7 @@ const useStyles = () => { | |||
| return {styles, colors} as const | |||
| } | |||
|
|
|||
| const mapAddresses = (addresses: {unused: string[]; used: string[]}): AddressInfo[] => { | |||
| const toAddressInfos = (addresses: {unused: string[]; used: string[]}): AddressInfo[] => { | |||
There was a problem hiding this comment.
transformer, was leaking impl
stackchain
commented
Feb 26, 2024
| import {useStrings} from '../common/useStrings' | ||
|
|
||
| export const EnterAmountScreen = () => { | ||
| export const RequestSpecificAmountScreen = () => { |
stackchain
commented
Feb 26, 2024
|
|
||
| const screenHeight = useWindowDimensions().height |
stackchain
commented
Feb 27, 2024
| @@ -83,13 +79,11 @@ export const EnterAmountScreen = () => { | |||
| ) | |||
| } | |||
|
|
|||
| const Modal = ({amount, address}: {amount: string; address?: string}) => { | |||
| const Modal = ({amount, address}: {amount: string; address: string}) => { | |||
stackchain
commented
Feb 27, 2024
| const {setMultipleAddressesOn, setMultipleAddressesOff, isToggleReceiveTypeLoading} = useMultipleAddresses() | ||
| const [isLocalMultipleAddressOff, setIsLocalMultipleAddressesOff] = React.useState(isMultipleAddressesOff) | ||
| const AddresseDerivationSwitcher = (props: {isSingle: boolean}) => { | ||
| const addressDerivation = useAddressDerivationManager() |
There was a problem hiding this comment.
refactor mainly for the new hooks
stackchain
commented
Feb 27, 2024
| @@ -33,6 +33,8 @@ const transferReducer = (state: TransferState, action: TransferAction) => { | |||
| draft.selectedTokenId = defaultTransferState.selectedTokenId | |||
| draft.memo = defaultTransferState.memo | |||
| draft.unsignedTx = castDraft(defaultTransferState.unsignedTx) | |||
| draft.selectedTargetIndex = defaultTransferState.selectedTargetIndex | |||
There was a problem hiding this comment.
bug when resetting state, was not setting back some properties cc: @banklesss (maybe there is a better way of settings the entire draft as initialState)
stackchain
commented
Feb 27, 2024
| entry: { | ||
| address: '', | ||
| amounts: {}, | ||
| export const defaultTransferState: TransferState = freeze( |
stackchain
requested review from
SorinC6,
michaeljscript,
banklesss and
jorbuedo
michaeljscript
approved these changes
Feb 27, 2024
Comment on lines
87
to
88
| singleMode: () => setAddressDerivationMode('single'), | ||
| multipleMode: () => setAddressDerivationMode('multiple'), |
There was a problem hiding this comment.
I'd suggest enableSingleMode and enableMultipleMode
|
share functionality will be refactored here https://emurgo.atlassian.net/browse/YOMO-1201 |
SorinC6
approved these changes
Feb 27, 2024
banklesss
approved these changes
Feb 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.