Skip to content

refactor(ramps): use bottom sheet modal for ramp region selection #20558

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
+459 −315
Closed

refactor(ramps): use bottom sheet modal for ramp region selection #20558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35ad88e
Revert "fix(deposit): fixes android region select bug (#17284)"
georgeweiler Sep 29, 2025
7364995
Merge branch 'main' of github.com:MetaMask/metamask-mobile
georgeweiler Sep 29, 2025
6a09c70
refactor(ramps): use bottom sheet modal for ramp region selection
georgeweiler Sep 29, 2025
8206458
chore: fixes tests
georgeweiler Sep 30, 2025
a7cf186
Merge branch 'main' of github.com:MetaMask/metamask-mobile into aggre…
georgeweiler Sep 30, 2025
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
16 changes: 8 additions & 8 deletions app/components/UI/Ramp/Aggregator/Views/BuildQuote/BuildQuote.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,13 @@ let mockUseBalanceValues: Partial<ReturnType<typeof useBalance>> = {

jest.mock('../../hooks/useBalance', () => jest.fn(() => mockUseBalanceValues));

const mockSetSelectedRegion = jest.fn();
const mockSetSelectedPaymentMethodId = jest.fn();
const mockSetSelectedAsset = jest.fn();
const mockSetSelectedFiatCurrencyId = jest.fn();

const mockUseRampSDKInitialValues: Partial<RampSDK> = {
selectedPaymentMethodId: mockPaymentMethods[0].id,
selectedRegion: mockRegionsData[0],
setSelectedRegion: mockSetSelectedRegion,
selectedAsset: mockCryptoCurrenciesData[0],
setSelectedAsset: mockSetSelectedAsset,
selectedFiatCurrencyId: mockFiatCurrenciesData[0].id,
Expand Down Expand Up @@ -324,7 +322,6 @@ describe('BuildQuote View', () => {
mockReset.mockClear();
mockPop.mockClear();
mockTrackEvent.mockClear();
(mockUseRampSDKInitialValues.setSelectedRegion as jest.Mock).mockClear();
jest.clearAllMocks();
});

Expand Down Expand Up @@ -504,17 +501,20 @@ describe('BuildQuote View', () => {
expect(mockQueryGetCountries).toBeCalledTimes(1);
});

it('calls setSelectedRegion when selecting a region', async () => {
it('navigates to region selector modal when region button is pressed', async () => {
render(BuildQuote);
await act(async () =>
fireEvent.press(
getByRoleButton(mockUseRegionsValues.selectedRegion?.emoji),
),
);
await act(async () =>
fireEvent.press(getByRoleButton(mockRegionsData[1].name)),
);
expect(mockSetSelectedRegion).toHaveBeenCalledWith(mockRegionsData[1]);

expect(mockNavigate).toHaveBeenCalledWith('RampModals', {
screen: 'RampRegionSelectorModal',
params: {
regions: mockRegionsData,
},
});
});
});

Expand Down
94 changes: 41 additions & 53 deletions app/components/UI/Ramp/Aggregator/Views/BuildQuote/BuildQuote.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ import PaymentMethodModal from '../../components/PaymentMethodModal';
import PaymentMethodIcon from '../../components/PaymentMethodIcon';
import FiatSelectModal from '../../components/modals/FiatSelectModal';
import ErrorViewWithReporting from '../../components/ErrorViewWithReporting';
import RegionModal from '../../components/RegionModal';
import SkeletonText from '../../components/SkeletonText';
import ErrorView from '../../components/ErrorView';
import BadgeWrapper, {
Expand All @@ -72,7 +71,8 @@ import {
import { createQuotesNavDetails } from '../Quotes/Quotes';
import { createTokenSelectModalNavigationDetails } from '../../components/TokenSelectModal/TokenSelectModal';
import { createIncompatibleAccountTokenModalNavigationDetails } from '../../components/IncompatibleAccountTokenModal';
import { QuickAmount, Region, ScreenLocation } from '../../types';
import { createRegionSelectorModalNavigationDetails } from '../../components/RegionSelectorModal';
import { QuickAmount, ScreenLocation } from '../../types';
import { useStyles } from '../../../../../../component-library/hooks';

import {
Expand Down Expand Up @@ -141,8 +141,6 @@ const BuildQuote = () => {
showPaymentMethodsModal,
hidePaymentMethodModal,
] = useModalHandler(false);
const [isRegionModalVisible, toggleRegionModal, , hideRegionModal] =
useModalHandler(false);

const nativeSymbol = useSelector(selectTicker);
const networkConfigurationsByCaipChainId = useSelector(
Expand All @@ -156,7 +154,6 @@ const BuildQuote = () => {
selectedPaymentMethodId,
setSelectedPaymentMethodId,
selectedRegion,
setSelectedRegion,
selectedAsset,
selectedFiatCurrencyId,
setSelectedFiatCurrencyId,
Expand Down Expand Up @@ -229,6 +226,37 @@ const BuildQuote = () => {
currentFiatCurrency,
);

useEffect(() => {
setAmount('0');
setAmountNumber(0);
}, [selectedRegion]);

useEffect(() => {
const handleRegionChange = async () => {
if (
selectedRegion &&
selectedFiatCurrencyId === defaultFiatCurrency?.id
) {
const newRegionCurrency = await queryDefaultFiatCurrency(
selectedRegion.id,
selectedPaymentMethodId ? [selectedPaymentMethodId] : null,
);
if (newRegionCurrency?.id) {
setSelectedFiatCurrencyId(newRegionCurrency.id);
}
}
};

handleRegionChange();
}, [
selectedRegion,
selectedFiatCurrencyId,
defaultFiatCurrency?.id,
queryDefaultFiatCurrency,
selectedPaymentMethodId,
setSelectedFiatCurrencyId,
]);
Copy link

Choose a reason for hiding this comment

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

Bug: Infinite Loop in useEffect Dependencies

The useEffect (lines 229-258) includes selectedFiatCurrencyId in its dependencies and updates it, which can lead to an infinite loop or redundant executions. This also introduces a race condition with queryDefaultFiatCurrency and triggers fiat currency updates on mount or other dependency changes, unlike the previous explicit region selection.

Fix in Cursor Fix in Web


const gasLimitEstimation = useERC20GasLimitEstimation({
tokenAddress: selectedAsset?.address,
fromAddress: selectedAddress,
Expand Down Expand Up @@ -509,37 +537,14 @@ const BuildQuote = () => {

const handleChangeRegion = useCallback(() => {
setAmountFocused(false);
toggleRegionModal();
}, [toggleRegionModal]);

const handleRegionPress = useCallback(
async (region: Region) => {
hideRegionModal();
setAmount('0');
setAmountNumber(0);
if (selectedFiatCurrencyId === defaultFiatCurrency?.id) {
/*
* Selected fiat currency is default, we will fetch
* and select new region default fiat currency
*/
const newRegionCurrency = await queryDefaultFiatCurrency(
region.id,
selectedPaymentMethodId ? [selectedPaymentMethodId] : null,
);
setSelectedFiatCurrencyId(newRegionCurrency?.id);
}
setSelectedRegion(region);
},
[
defaultFiatCurrency?.id,
hideRegionModal,
queryDefaultFiatCurrency,
selectedFiatCurrencyId,
selectedPaymentMethodId,
setSelectedFiatCurrencyId,
setSelectedRegion,
],
);
if (regions) {
navigation.navigate(
...createRegionSelectorModalNavigationDetails({
regions,
}),
);
}
}, [navigation, regions]);

/**
* * CryptoCurrency handlers
Expand Down Expand Up @@ -1112,23 +1117,6 @@ const BuildQuote = () => {
location={screenLocation}
rampType={rampType}
/>
{regions && (
<RegionModal
isVisible={isRegionModalVisible}
title={strings('fiat_on_ramp_aggregator.region.title')}
description={strings(
isBuy
? 'fiat_on_ramp_aggregator.region.description'
: 'fiat_on_ramp_aggregator.region.sell_description',
)}
data={regions}
dismiss={hideRegionModal as () => void}
onRegionPress={handleRegionPress}
location={screenLocation}
selectedRegion={selectedRegion}
rampType={rampType}
/>
)}
</ScreenLayout>
);
};
Expand Down
7 changes: 0 additions & 7 deletions app/components/UI/Ramp/Aggregator/Views/BuildQuote/__snapshots__/BuildQuote.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4493,7 +4493,6 @@ exports[`BuildQuote View Crypto Currency Data renders the loading page when cryp
</View>
<View />
<View />
<View />
</View>
</RNCSafeAreaView>
</View>
Expand Down Expand Up @@ -7795,7 +7794,6 @@ exports[`BuildQuote View Fiat Currency Data renders the loading page when fiats
</View>
<View />
<View />
<View />
</View>
</RNCSafeAreaView>
</View>
Expand Down Expand Up @@ -11142,7 +11140,6 @@ exports[`BuildQuote View Payment Method Data renders no icons if there are no pa
</View>
<View />
<View />
<View />
</View>
</RNCSafeAreaView>
</View>
Expand Down Expand Up @@ -13767,7 +13764,6 @@ exports[`BuildQuote View Payment Method Data renders the loading page when payme
</View>
<View />
<View />
<View />
</View>
</RNCSafeAreaView>
</View>
Expand Down Expand Up @@ -16734,7 +16730,6 @@ exports[`BuildQuote View Regions data renders the loading page when regions are
</View>
<View />
<View />
<View />
</View>
</RNCSafeAreaView>
</View>
Expand Down Expand Up @@ -19483,7 +19478,6 @@ exports[`BuildQuote View renders correctly 1`] = `
</View>
<View />
<View />
<View />
</View>
</RNCSafeAreaView>
</View>
Expand Down Expand Up @@ -22212,7 +22206,6 @@ exports[`BuildQuote View renders correctly 2`] = `
</View>
<View />
<View />
<View />
</View>
</RNCSafeAreaView>
</View>
Expand Down
25 changes: 25 additions & 0 deletions ...omponents/UI/Ramp/Aggregator/components/RegionSelectorModal/RegionSelectorModal.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { StyleSheet } from 'react-native';

const createStyles = () =>
StyleSheet.create({
Copy link

Choose a reason for hiding this comment

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

Bug: Styles Function Fails to Accept Parameters

The createStyles function in RegionSelectorModal.styles.ts doesn't accept parameters, but useStyles passes { theme, vars }. This prevents theme and variables like screenHeight from being used in the styles, potentially causing runtime errors.

Fix in Cursor Fix in Web

searchContainer: {
paddingHorizontal: 16,
paddingBottom: 16,
},
list: {
flex: 1,
},
region: {
flexDirection: 'row',
alignItems: 'center',
},
emoji: {
paddingRight: 16,
},
emptyList: {
padding: 16,
alignItems: 'center',
},
});

export default createStyles;
Loading
Loading