Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
192 changes: 88 additions & 104 deletions ui/pages/bridge/prepare/bridge-transaction-settings-modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@ import React, { useState, useEffect } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { BRIDGE_DEFAULT_SLIPPAGE } from '@metamask/bridge-controller';
import {
Button,
ButtonPrimary,
ButtonPrimarySize,
ButtonSize,
ButtonVariant,
ButtonSecondary,
ButtonSecondarySize,
Modal,
ModalContent,
ModalFooter,
Expand All @@ -19,17 +18,16 @@ import {
BannerAlert,
BannerAlertSeverity,
Box,
TextFieldSize,
} from '../../../components/component-library';
import { useI18nContext } from '../../../hooks/useI18nContext';
import {
BackgroundColor,
BlockSize,
BorderColor,
BorderRadius,
JustifyContent,
TextColor,
TextVariant,
SEVERITIES,
BorderRadius,
} from '../../../helpers/constants/design-system';
import { getIsSolanaSwap, getSlippage } from '../../../ducks/bridge/selectors';
import { setSlippage } from '../../../ducks/bridge/actions';
Expand Down Expand Up @@ -109,127 +107,114 @@ export const BridgeTransactionSettingsModal = ({
<ModalOverlay />
<ModalContent>
<ModalHeader onClose={onClose}>{t('transactionSettings')}</ModalHeader>
<Column gap={3} padding={4}>
<Column gap={3} paddingInline={4} paddingBottom={4}>
<Row gap={1} justifyContent={JustifyContent.flexStart}>
<Text>{t('swapsMaxSlippage')}</Text>
<Text variant={TextVariant.bodyMdMedium}>
{t('swapsMaxSlippage')}
</Text>
<Tooltip position={PopoverPosition.Top} style={{ zIndex: 1051 }}>
{t('swapSlippageTooltip')}
</Tooltip>
</Row>
<Row gap={2} justifyContent={JustifyContent.flexStart}>
{shouldShowAutoOption && (
<Button
size={ButtonSize.Sm}
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
e.stopPropagation();
setLocalSlippage(undefined);
setCustomSlippage(undefined);
setIsAutoSelected(true);
}}
variant={ButtonVariant.Secondary}
borderColor={
isAutoSelected
? BorderColor.primaryDefault
: BorderColor.borderDefault
}
borderWidth={isAutoSelected ? 2 : 1}
backgroundColor={
isAutoSelected
? BackgroundColor.primaryMuted
: BackgroundColor.backgroundDefault
}
>
<Text
color={
isAutoSelected
? TextColor.primaryDefault
: TextColor.textDefault
}
>
{t('swapSlippageAutoDescription')}
</Text>
</Button>
<>
{isAutoSelected ? (
<ButtonPrimary
size={ButtonPrimarySize.Md}
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
e.stopPropagation();
setLocalSlippage(undefined);
setCustomSlippage(undefined);
setIsAutoSelected(true);
}}
>
{t('swapSlippageAutoDescription')}
</ButtonPrimary>
) : (
<ButtonSecondary
size={ButtonSecondarySize.Md}
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
e.stopPropagation();
setLocalSlippage(undefined);
setCustomSlippage(undefined);
setIsAutoSelected(true);
}}
>
{t('swapSlippageAutoDescription')}
</ButtonSecondary>
)}
</>
)}
{HARDCODED_SLIPPAGE_OPTIONS.map((hardcodedSlippage) => {
const isSelected =
!isAutoSelected && localSlippage === hardcodedSlippage;
return (
<Button
isAutoSelected === false && localSlippage === hardcodedSlippage;
Copy link

Choose a reason for hiding this comment

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

Bug: Slippage Button Selection Logic Bug

The selection logic for hardcoded slippage buttons now explicitly checks for isAutoSelected === false instead of any falsy value. This can cause hardcoded slippage options to appear unselected if isAutoSelected is undefined during initialization or state updates.

Fix in Cursor Fix in Web

Copy link
Member

Choose a reason for hiding this comment

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

@amandaye0h I agree with Cursor here. can you change this to check for !isAutoSelected instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, I reverted the change:

Screenshot 2025-10-23 at 2 24 01 AM

return isSelected ? (
<ButtonPrimary
key={hardcodedSlippage}
size={ButtonSize.Sm}
size={ButtonPrimarySize.Md}
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
e.stopPropagation();
setLocalSlippage(hardcodedSlippage);
setCustomSlippage(undefined);
setIsAutoSelected(false);
}}
variant={ButtonVariant.Secondary}
borderColor={
isSelected
? BorderColor.primaryDefault
: BorderColor.borderDefault
}
borderWidth={isSelected ? 2 : 1}
backgroundColor={
isSelected
? BackgroundColor.primaryMuted
: BackgroundColor.backgroundDefault
}
>
<Text
color={
isSelected
? TextColor.primaryDefault
: TextColor.textDefault
}
>
{hardcodedSlippage}%
</Text>
</Button>
{hardcodedSlippage}%
</ButtonPrimary>
) : (
<ButtonSecondary
key={hardcodedSlippage}
size={ButtonSecondarySize.Md}
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
e.stopPropagation();
setLocalSlippage(hardcodedSlippage);
setCustomSlippage(undefined);
setIsAutoSelected(false);
}}
>
{hardcodedSlippage}%
</ButtonSecondary>
);
})}
{showCustomButton && (
<Button
size={ButtonSize.Sm}
variant={ButtonVariant.Secondary}
borderColor={
customSlippage === undefined
? BorderColor.borderDefault
: BorderColor.primaryDefault
}
borderWidth={customSlippage === undefined ? 1 : 2}
backgroundColor={
customSlippage === undefined
? BackgroundColor.backgroundDefault
: BackgroundColor.primaryMuted
}
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
e.stopPropagation();
setShowCustomButton(false);
setIsAutoSelected(false);
}}
>
<Text
color={
customSlippage === undefined
? TextColor.textDefault
: TextColor.primaryDefault
}
>
{customSlippage === undefined
? t('customSlippage')
: `${customSlippage}%`}
</Text>
</Button>
<>
Copy link
Contributor

@infiniteflower infiniteflower Oct 8, 2025

Choose a reason for hiding this comment

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

Can we not use the variant prop way so we can prevent duplicating logic by having only a single button component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, thanks for the suggestion!

I've updated the code to conditionally render the buttons using the variant prop. Now the logic is not duplicated.

{customSlippage === undefined ? (
<ButtonSecondary
size={ButtonSecondarySize.Md}
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
e.stopPropagation();
setShowCustomButton(false);
setIsAutoSelected(false);
}}
>
{t('customSlippage')}
</ButtonSecondary>
) : (
<ButtonPrimary
size={ButtonPrimarySize.Md}
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
e.stopPropagation();
setShowCustomButton(false);
setIsAutoSelected(false);
}}
>
{`${customSlippage}%`}
</ButtonPrimary>
)}
</>
)}
{!showCustomButton && (
<TextField
borderColor={BorderColor.primaryDefault}
borderWidth={2}
borderRadius={BorderRadius.pill}
size={TextFieldSize.Md}
borderColor={BorderColor.borderMuted}
borderRadius={BorderRadius.XL}
type={TextFieldType.Text}
value={customSlippage}
onChange={(e) => {
Expand Down Expand Up @@ -266,8 +251,7 @@ export const BridgeTransactionSettingsModal = ({
<ModalFooter>
<ButtonPrimary
width={BlockSize.Full}
size={ButtonPrimarySize.Md}
variant={TextVariant.bodyMd}
size={ButtonPrimarySize.Lg}
disabled={(() => {
// Calculate what the new slippage would be
const newSlippage = isAutoSelected
Expand Down
4 changes: 0 additions & 4 deletions ui/pages/bridge/prepare/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@
}

.mm-text-field {
height: 32px;
width: 94px;

&--focused,
Expand All @@ -111,10 +110,7 @@
}

input {
font-size: var(--font-size-2);
padding-top: 1px;
width: 100%;
height: 32px;
}
}
}
Loading