Skip to content
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

fix: APP-410 save buy flow on last user interaction #2574

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import {
spendingCapAtom,
} from 'pages/BuyCredits/BuyCredits.atoms';
import { PAYMENT_OPTIONS } from 'pages/BuyCredits/BuyCredits.constants';
import { BuyCreditsSchemaTypes } from 'pages/BuyCredits/BuyCredits.types';
import { getCreditsAvailableBannerText } from 'pages/BuyCredits/BuyCredits.utils';
import { useMultiStep } from 'components/templates/MultiStepTemplate';

import { findDisplayDenom } from '../DenomLabel/DenomLabel.utils';
import {
Expand Down Expand Up @@ -53,6 +55,11 @@ export const CreditsAmount = ({
const setErrorBannerTextAtom = useSetAtom(errorBannerTextAtom);
const [spendingCap, setSpendingCap] = useAtom(spendingCapAtom);
const paymentOption = useAtomValue(paymentOptionAtom);
const {
data,
handleSave: updateMultiStepData,
activeStep,
} = useMultiStep<BuyCreditsSchemaTypes>();

useEffect(() => {
// Set initial credits amount to min(1, creditsAvailable)
Expand Down Expand Up @@ -136,6 +143,16 @@ export const CreditsAmount = ({
setErrorBannerTextAtom(
getCreditsAvailableBannerText(_creditsAvailable),
);
// Udates the currency amount in multistep data when payment option is changed
updateMultiStepData(
{
...data,
creditsAmount: _creditsAvailable,
currencyAmount: _spendingCap,
sellOrders: orderedSellOrders,
Copy link
Member

Choose a reason for hiding this comment

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

why aren't we setting this to the same value as in setValue(SELL_ORDERS, ...) just above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good you noticed this, thanks

},
activeStep,
);
} else {
// Else we keep the same amount of credits
// but we still need to update currency amount and sell orders
Expand All @@ -148,8 +165,21 @@ export const CreditsAmount = ({
});
setValue(CURRENCY_AMOUNT, currencyAmount);
setValue(SELL_ORDERS, sellOrders);
// Updates the currency amount in multistep data when payment option is changed
updateMultiStepData(
{
...data,
creditsAmount: currentCreditsAmount,
currencyAmount: currencyAmount,
sellOrders: sellOrders,
},
activeStep,
);
}
}
// Intentionally omit `updateMultiStepData` and `data` from the dependency array
// because including them trigger unnecessary renders.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [
cardSellOrders,
filteredCryptoSellOrders,
Expand All @@ -163,6 +193,7 @@ export const CreditsAmount = ({
card,
setErrorBannerTextAtom,
_,
activeStep,
]);

// Max credits set
Expand All @@ -172,26 +203,35 @@ export const CreditsAmount = ({
setValue(CURRENCY_AMOUNT, spendingCap, {
shouldValidate: true,
});
setValue(
SELL_ORDERS,
orderedSellOrders.map(order => {
const price = getSellOrderPrice({ order, card });
return formatSellOrder({ order, card, price });
}),
const sellOrders = orderedSellOrders.map(order => {
const price = getSellOrderPrice({ order, card });
return formatSellOrder({ order, card, price });
});
setValue(SELL_ORDERS, sellOrders);
updateMultiStepData(
{
...data,
creditsAmount: creditsAvailable,
currencyAmount: spendingCap,
sellOrders: sellOrders,
},
activeStep,
);
setMaxCreditsSelected(false);
}
}, [
activeStep,
card,
creditsAvailable,
data,
maxCreditsSelected,
orderedSellOrders,
paymentOption,
setValue,
spendingCap,
updateMultiStepData,
]);

// Credits amount change
const handleCreditsAmountChange = useCallback(
(e: ChangeEvent<HTMLInputElement>) => {
// Set currency amount according to credits quantity,
Expand All @@ -205,11 +245,21 @@ export const CreditsAmount = ({
});
setValue(CURRENCY_AMOUNT, currencyAmount, { shouldValidate: true });
setValue(SELL_ORDERS, sellOrders);
updateMultiStepData(
{
...data,
creditsAmount: currentCreditsAmount,
currencyAmount: currencyAmount,
Copy link
Member

Choose a reason for hiding this comment

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

why not updating sellOrders too?

},
activeStep,
);
},
[card, orderedSellOrders, setValue, creditTypePrecision],
// Intentionally omit `updateMultiStepData` and `data` from the dependency array
// because including them trigger unnecessary renders.
// eslint-disable-next-line react-hooks/exhaustive-deps
[card, orderedSellOrders, creditTypePrecision, setValue, activeStep],
);

// Currency amount change
const handleCurrencyAmountChange = useCallback(
(e: ChangeEvent<HTMLInputElement>) => {
// Set credits quantity according to currency amount,
Expand All @@ -221,10 +271,22 @@ export const CreditsAmount = ({
orderedSellOrders,
creditTypePrecision,
});

updateMultiStepData(
{
...data,
creditsAmount: currentCreditsAmount,
currencyAmount: isNaN(value) ? 0 : value,
Copy link
Member

Choose a reason for hiding this comment

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

ditto

},
activeStep,
);
setValue(CREDITS_AMOUNT, currentCreditsAmount, { shouldValidate: true });
setValue(SELL_ORDERS, sellOrders);
},
[card, orderedSellOrders, setValue, creditTypePrecision],
// Intentionally omit `updateMultiStepData` and `data` from the dependency array
// because including them trigger unnecessary renders.
// eslint-disable-next-line react-hooks/exhaustive-deps
[card, orderedSellOrders, creditTypePrecision, setValue],
);

const displayDenom = useMemo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import TextField from 'web-components/src/components/inputs/new/TextField/TextFi
import { paymentOptionAtom } from 'pages/BuyCredits/BuyCredits.atoms';
import { PAYMENT_OPTIONS } from 'pages/BuyCredits/BuyCredits.constants';
import { BuyCreditsSchemaTypes } from 'pages/BuyCredits/BuyCredits.types';
import { updateMultiStepCurrencyAndPaymentOption } from 'pages/BuyCredits/BuyCredits.utils';
import { DenomIconWithCurrency } from 'components/molecules/DenomIconWithCurrency/DenomIconWithCurrency';
import { useMultiStep } from 'components/templates/MultiStepTemplate';

Expand Down Expand Up @@ -43,8 +42,11 @@ export const CurrencyInput = ({
control,
} = useFormContext<ChooseCreditsFormSchemaType>();
const { _ } = useLingui();
const { data, handleSave, activeStep } =
useMultiStep<BuyCreditsSchemaTypes>();
const {
data,
handleSave: updateMultiStepData,
activeStep,
} = useMultiStep<BuyCreditsSchemaTypes>();
const paymentOption = useAtomValue(paymentOptionAtom);
const card = paymentOption === PAYMENT_OPTIONS.CARD;
const { onChange } = register(CURRENCY_AMOUNT);
Expand Down Expand Up @@ -97,9 +99,18 @@ export const CurrencyInput = ({
const value = event.target.value;
if (value === '') {
setValue(CURRENCY_AMOUNT, 0, { shouldValidate: true });
updateMultiStepData(
{
...data,
currency,
paymentOption,
currencyAmount: 0,
},
activeStep,
);
Comment on lines +102 to +110
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?
we update the currencyAmount but not the sell orders which depend on it, which could lead to inconsistent data in local storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm updating the multistep data here to keep it in sync with the input field, don't you think this needs to be done?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure since it seems like this will be updated anyway from handleCurrencyAmountChange with the correct values for corresponding sell orders and credits input

}
},
[setValue],
[activeStep, currency, data, paymentOption, setValue, updateMultiStepData],
);

const onHandleCurrencyChange = useCallback(
Expand All @@ -114,15 +125,23 @@ export const CurrencyInput = ({
)?.[0].askBaseDenom,
};
setValue(CURRENCY, currency);
updateMultiStepCurrencyAndPaymentOption(
handleSave,
data,
currency,
updateMultiStepData(
{
...data,
currency,
paymentOption,
},
activeStep,
paymentOption,
);
},
[activeStep, cryptoCurrencies, data, handleSave, paymentOption, setValue],
[
cryptoCurrencies,
setValue,
updateMultiStepData,
data,
paymentOption,
activeStep,
],
);

return (
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { ChangeEvent } from 'react';
import { Trans } from '@lingui/macro';
import { useLingui } from '@lingui/react';
import { cryptoOptions } from 'web-marketplace/src/components/molecules/CreditsAmount/CreditsAmount.constants';
Expand All @@ -12,7 +13,7 @@ export function CryptoOptions({
tradableDisabled = false,
}: {
retiring: boolean;
handleCryptoPurchaseOptions: () => void;
handleCryptoPurchaseOptions: (event: ChangeEvent<HTMLInputElement>) => void;
tradableDisabled?: boolean;
}) {
const { _ } = useLingui();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, {
ChangeEvent,
startTransition,
useCallback,
useEffect,
Expand Down Expand Up @@ -44,7 +45,6 @@ import {
import {
getCreditsAvailableBannerText,
getCryptoCurrencies,
updateMultiStepCurrencyAndPaymentOption,
} from 'pages/BuyCredits/BuyCredits.utils';
import {
ProjectWithOrderData,
Expand Down Expand Up @@ -116,8 +116,11 @@ export const ChooseCreditsForm = React.memo(
}: Props) => {
const { _ } = useLingui();
const setErrorBannerTextAtom = useSetAtom(errorBannerTextAtom);
const { data, handleSave, activeStep } =
useMultiStep<BuyCreditsSchemaTypes>();
const {
data,
handleSave: updateMultiStepData,
activeStep,
} = useMultiStep<BuyCreditsSchemaTypes>();
const [paymentOption, setPaymentOption] = useAtom(paymentOptionAtom);

const cryptoCurrencies = useMemo(
Expand Down Expand Up @@ -201,9 +204,19 @@ export const ChooseCreditsForm = React.memo(
[card, cardSellOrders, filteredCryptoSellOrders],
);

const handleCryptoPurchaseOptions = useCallback(() => {
setRetiring(prev => !prev);
}, [setRetiring]);
const handleCryptoPurchaseOptions = useCallback(
(event: ChangeEvent<HTMLInputElement>) => {
setRetiring(prev => !prev);
updateMultiStepData(
{
...data,
retiring: event.target.value === 'true',
},
activeStep,
);
},
[activeStep, data, setRetiring, updateMultiStepData],
);

const handlePaymentOptions = useCallback(
(option: string) => {
Expand All @@ -219,22 +232,23 @@ export const ChooseCreditsForm = React.memo(
[CREDIT_VINTAGE_OPTIONS]: [],
[CURRENCY]: currency,
});
updateMultiStepCurrencyAndPaymentOption(
handleSave,
data,
currency,
updateMultiStepData(
{
...data,
currency,
paymentOption: option as PaymentOptionsType,
},
activeStep,
option,
);
},
[
form,
activeStep,
cardCurrency,
defaultCryptoCurrency,
handleSave,
data,
activeStep,
defaultCryptoCurrency,
form,
setPaymentOption,
updateMultiStepData,
],
);

Expand All @@ -244,12 +258,13 @@ export const ChooseCreditsForm = React.memo(
setPaymentOption(PAYMENT_OPTIONS.CARD);
form.setValue(CREDIT_VINTAGE_OPTIONS, []);
form.setValue(CURRENCY, cardCurrency);
updateMultiStepCurrencyAndPaymentOption(
handleSave,
data,
cardCurrency,
updateMultiStepData(
{
...data,
currency: cardCurrency,
paymentOption: PAYMENT_OPTIONS.CARD,
},
activeStep,
PAYMENT_OPTIONS.CARD,
);
}
}, [cardSellOrders.length, initialPaymentOption]); // just run this once
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import CheckboxLabel from 'web-components/src/components/inputs/new/CheckboxLabe
import { Body } from 'web-components/src/components/typography';
import { UseStateSetter } from 'web-components/src/types/react/useState';

import { useMultiStep } from 'components/templates/MultiStepTemplate';

import { paymentElementOptions } from './PaymentInfoForm.constants';
import { PaymentInfoFormSchemaType } from './PaymentInfoForm.schema';

Expand All @@ -22,6 +24,7 @@ export const CardInfo = ({
}: CardInfoProps) => {
const ctx = useFormContext<PaymentInfoFormSchemaType>();
const { register, control, setValue } = ctx;
const { handleSave: updateMultiStepData, activeStep, data } = useMultiStep();

const createAccount = useWatch({
control: control,
Expand All @@ -36,6 +39,19 @@ export const CardInfo = ({
setValue('savePaymentMethod', createAccount);
}, [createAccount, setValue]);

useEffect(() => {
updateMultiStepData(
{
...data,
savePaymentMethod,
},
activeStep,
);
// Intentionally omit `updateMultiStepData` and `data` from the dependency array
// because including them trigger unnecessary renders.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [savePaymentMethod, activeStep]);

return (
<div className={className}>
<PaymentElement
Expand Down
Loading
Loading