-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update the Assigned Cards Section of the Wallet Page #38998
Changes from 5 commits
0515ffb
ecdcf48
804005a
71c86a2
13674cd
e5d8cc5
a26532f
ecd7c73
6bf60e9
a10788e
bc9ce69
c19eb59
ae6f359
24321a9
70d02e4
55bed2d
5b9ad8c
a8454dc
4f6ee19
0486463
f989cea
f114894
d542600
c7a0f12
0458752
f915905
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,11 +111,26 @@ type SettingsNavigatorParamList = { | |
}; | ||
[SCREENS.SETTINGS.WALLET.ROOT]: undefined; | ||
[SCREENS.SETTINGS.WALLET.CARDS_DIGITAL_DETAILS_UPDATE_ADDRESS]: undefined; | ||
[SCREENS.SETTINGS.WALLET.DOMAIN_CARD]: undefined; | ||
[SCREENS.SETTINGS.WALLET.REPORT_VIRTUAL_CARD_FRAUD]: undefined; | ||
[SCREENS.SETTINGS.WALLET.CARD_ACTIVATE]: undefined; | ||
[SCREENS.SETTINGS.WALLET.DOMAIN_CARD]: { | ||
/** domain passed via route /settings/wallet/card/:domain/:card */ | ||
domain: string; | ||
/** cardId passed via route /settings/wallet/card/:domain/:card */ | ||
cardId: string; | ||
}; | ||
[SCREENS.SETTINGS.WALLET.REPORT_VIRTUAL_CARD_FRAUD]: { | ||
/** domain passed via route /settings/wallet/card/:domain/:card */ | ||
domain: string; | ||
/** cardId passed via route /settings/wallet/card/:domain/:card */ | ||
cardId: string; | ||
}; | ||
[SCREENS.SETTINGS.WALLET.CARD_ACTIVATE]: { | ||
/** domain passed via route /settings/wallet/card/:domain/:card */ | ||
domain: string; | ||
/** cardId passed via route /settings/wallet/card/:domain/:card */ | ||
cardId: string; | ||
}; | ||
[SCREENS.SETTINGS.WALLET.CARD_GET_PHYSICAL.NAME]: { | ||
/** domain passed via route /settings/wallet/card/:domain */ | ||
/** domain passed via route /settings/wallet/card/:domain/:card */ | ||
domain: string; | ||
}; | ||
[SCREENS.SETTINGS.WALLET.CARD_GET_PHYSICAL.PHONE]: { | ||
|
@@ -250,7 +265,12 @@ type SettingsNavigatorParamList = { | |
backTo: Routes; | ||
}; | ||
[SCREENS.SETTINGS.TWO_FACTOR_AUTH]: BackToParams; | ||
[SCREENS.SETTINGS.REPORT_CARD_LOST_OR_DAMAGED]: undefined; | ||
[SCREENS.SETTINGS.REPORT_CARD_LOST_OR_DAMAGED]: { | ||
/** domain passed via route /settings/wallet/card/:domain/:card */ | ||
domain: string; | ||
/** cardId passed via route /settings/wallet/card/:domain/:card */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I understand this style of comment is already used in this file, I still find it kinda redundant. Do you really think its needed? and if yes then could we write it in some more human-friendly way? so something like |
||
cardId: string; | ||
}; | ||
[SCREENS.KEYBOARD_SHORTCUTS]: undefined; | ||
[SCREENS.SETTINGS.EXIT_SURVEY.REASON]: undefined; | ||
[SCREENS.SETTINGS.EXIT_SURVEY.RESPONSE]: { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ import * as CardUtils from '@libs/CardUtils'; | |
import * as DeviceCapabilities from '@libs/DeviceCapabilities'; | ||
import * as ErrorUtils from '@libs/ErrorUtils'; | ||
import Navigation from '@libs/Navigation/Navigation'; | ||
import type {PublicScreensParamList} from '@libs/Navigation/types'; | ||
import type {SettingsNavigatorParamList} from '@libs/Navigation/types'; | ||
import NotFoundPage from '@pages/ErrorPage/NotFoundPage'; | ||
import * as CardSettings from '@userActions/Card'; | ||
import CONST from '@src/CONST'; | ||
|
@@ -34,15 +34,15 @@ type ActivatePhysicalCardPageOnyxProps = { | |
cardList: OnyxEntry<Record<string, Card>>; | ||
}; | ||
|
||
type ActivatePhysicalCardPageProps = ActivatePhysicalCardPageOnyxProps & StackScreenProps<PublicScreensParamList, typeof SCREENS.TRANSITION_BETWEEN_APPS>; | ||
type ActivatePhysicalCardPageProps = ActivatePhysicalCardPageOnyxProps & StackScreenProps<SettingsNavigatorParamList, typeof SCREENS.SETTINGS.WALLET.CARD_ACTIVATE>; | ||
grgia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const LAST_FOUR_DIGITS_LENGTH = 4; | ||
const MAGIC_INPUT_MIN_HEIGHT = 86; | ||
|
||
function ActivatePhysicalCardPage({ | ||
cardList, | ||
route: { | ||
params: {domain = ''}, | ||
params: {domain = '', cardId = ''}, | ||
}, | ||
}: ActivatePhysicalCardPageProps) { | ||
const theme = useTheme(); | ||
|
@@ -56,8 +56,7 @@ function ActivatePhysicalCardPage({ | |
const [lastPressedDigit, setLastPressedDigit] = useState(''); | ||
|
||
const domainCards = CardUtils.getDomainCards(cardList)[domain] ?? []; | ||
const physicalCard = domainCards.find((card) => !card.isVirtual); | ||
const cardID = physicalCard?.cardID ?? 0; | ||
const physicalCard = domainCards.find((card) => card?.state === CONST.EXPENSIFY_CARD.STATE.NOT_ACTIVATED); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don’t we have to also add the check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only physical Card can be in NOT_ACTIVATED state. I was also wondering about, ff it's more readable to add isVirtual check, but decided to skip it since it's redundant. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Got it. Since NOT_ACTIVATED state is only for physical card, it is fine not to add a redundant check. |
||
const cardError = ErrorUtils.getLatestErrorMessage(physicalCard ?? {}); | ||
|
||
const activateCardCodeInputRef = useRef<MagicCodeInputHandle>(null); | ||
|
@@ -66,19 +65,18 @@ function ActivatePhysicalCardPage({ | |
* If state of the card is CONST.EXPENSIFY_CARD.STATE.OPEN, navigate to card details screen. | ||
*/ | ||
useEffect(() => { | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
if (physicalCard?.isLoading || cardList?.[cardID]?.state !== CONST.EXPENSIFY_CARD.STATE.OPEN) { | ||
if (physicalCard?.state !== CONST.EXPENSIFY_CARD.STATE.OPEN || physicalCard?.isLoading) { | ||
return; | ||
} | ||
|
||
Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(domain)); | ||
}, [cardID, cardList, domain, physicalCard?.isLoading]); | ||
Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(domain, cardId)); | ||
}, [cardId, cardList, domain, physicalCard?.isLoading, physicalCard?.state]); | ||
|
||
useEffect( | ||
() => () => { | ||
CardSettings.clearCardListErrors(cardID); | ||
CardSettings.clearCardListErrors(physicalCard?.cardID ?? 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm do we want to default to 0 here? It looks like it would merge 0 into the cardList There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe not though? would 0: {} work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could just do if and an early return. Dunno why I didn't think of that. Defaulting to 0 would probably cause bugs 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah let's do an early return instead |
||
}, | ||
[cardID], | ||
[physicalCard?.cardID], | ||
); | ||
|
||
/** | ||
|
@@ -96,7 +94,7 @@ function ActivatePhysicalCardPage({ | |
setFormError(''); | ||
|
||
if (cardError) { | ||
CardSettings.clearCardListErrors(cardID); | ||
CardSettings.clearCardListErrors(physicalCard?.cardID ?? 0); | ||
} | ||
|
||
setLastFourDigits(text); | ||
|
@@ -110,8 +108,8 @@ function ActivatePhysicalCardPage({ | |
return; | ||
} | ||
|
||
CardSettings.activatePhysicalExpensifyCard(lastFourDigits, cardID); | ||
}, [lastFourDigits, cardID]); | ||
CardSettings.activatePhysicalExpensifyCard(lastFourDigits, physicalCard?.cardID ?? 0); | ||
}, [lastFourDigits, physicalCard?.cardID]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should default to a negative ID There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could but isn't 0 also a wrong card ID and would cause There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah 0 is an incorrect card ID. I think we've historically used -1 but I need to double check. But let me know if you'll try returning early instead |
||
|
||
if (isEmptyObject(physicalCard)) { | ||
return <NotFoundPage />; | ||
|
@@ -120,7 +118,7 @@ function ActivatePhysicalCardPage({ | |
return ( | ||
<IllustratedHeaderPageLayout | ||
title={translate('activateCardPage.activateCard')} | ||
onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(domain))} | ||
onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(domain, cardId))} | ||
backgroundColor={theme.PAGE_THEMES[SCREENS.SETTINGS.PREFERENCES.ROOT].backgroundColor} | ||
illustration={LottieAnimations.Magician} | ||
scrollViewContainerStyles={[styles.mnh100]} | ||
|
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.
Did we miss translation for the names i.e.
Smart limit
,Fixed limit
,Monthly limit
? Or was this intentional?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.
Asked @grgia about it 😄