-
Notifications
You must be signed in to change notification settings - Fork 145
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
chore: Update SwapSettingsSlippageInput to use Config #1263
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@coinbase/onchainkit': patch | ||
--- | ||
|
||
-**chore**: updated `SwapSettingsSlippageInput` to use the input config defaultMaxSlippage value. By @cpcramer #1263. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,47 +12,44 @@ const SLIPPAGE_SETTINGS = { | |
export function SwapSettingsSlippageInput({ | ||
className, | ||
}: SwapSettingsSlippageInputReact) { | ||
const { updateLifecycleStatus, lifecycleStatus } = useSwapContext(); | ||
const getMaxSlippage = useCallback(() => { | ||
return lifecycleStatus.statusData.maxSlippage; | ||
}, [lifecycleStatus.statusData]); | ||
const { | ||
config: { maxSlippage: defaultMaxSlippage }, | ||
updateLifecycleStatus, | ||
lifecycleStatus, | ||
} = useSwapContext(); | ||
|
||
// Set initial slippage values to match previous selection or default, | ||
// ensuring consistency when dropdown is reopened | ||
const [slippage, setSlippage] = useState(getMaxSlippage()); | ||
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. Not needed anymore |
||
const [slippageSetting, setSlippageSetting] = useState( | ||
getMaxSlippage() === DEFAULT_MAX_SLIPPAGE | ||
lifecycleStatus.statusData.maxSlippage === defaultMaxSlippage | ||
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 nice |
||
? SLIPPAGE_SETTINGS.AUTO | ||
: SLIPPAGE_SETTINGS.CUSTOM, | ||
); | ||
|
||
const updateSlippage = useCallback( | ||
(newSlippage: number) => { | ||
setSlippage(newSlippage); | ||
updateLifecycleStatus({ | ||
statusName: 'slippageChange', | ||
statusData: { | ||
maxSlippage: newSlippage, | ||
}, | ||
}); | ||
if (newSlippage !== lifecycleStatus.statusData.maxSlippage) { | ||
updateLifecycleStatus({ | ||
statusName: 'slippageChange', | ||
statusData: { | ||
maxSlippage: newSlippage, | ||
}, | ||
}); | ||
} | ||
}, | ||
[updateLifecycleStatus], | ||
[lifecycleStatus.statusData.maxSlippage, updateLifecycleStatus], | ||
); | ||
|
||
// Handles user input for custom slippage | ||
// Parses the input and updates slippage if valid | ||
// Handles user input for custom slippage. | ||
// Parses the input and updates slippage state. | ||
const handleSlippageChange = useCallback( | ||
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. Well done in updating the comments as well |
||
(newSlippage: string) => { | ||
// Empty '' when the input field is cleared. | ||
if (newSlippage === '') { | ||
setSlippage(0); | ||
return; | ||
} | ||
(e: React.ChangeEvent<HTMLInputElement>) => { | ||
const newSlippage = e.target.value; | ||
const parsedSlippage = Number.parseFloat(newSlippage); | ||
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. parseFloat('2.') = 2 preventing you from entering decimals 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 think this is a fast follow update. I belieeve we will need to update 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, parseFloat(2.2) = 2.2, its just typing the interim step will remove the dot before you type the next number 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. @cpcramer in your update to 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. yep! |
||
const isValidNumber = !Number.isNaN(parsedSlippage); | ||
|
||
const newSlippageNumber = Number.parseFloat(newSlippage); | ||
if (!Number.isNaN(newSlippageNumber)) { | ||
updateSlippage(newSlippageNumber); | ||
} | ||
// Update slippage to parsed value if valid, otherwise set to 0 | ||
updateSlippage(isValidNumber ? parsedSlippage : 0); | ||
}, | ||
[updateSlippage], | ||
); | ||
|
@@ -119,8 +116,8 @@ export function SwapSettingsSlippageInput({ | |
<input | ||
id="slippage-input" | ||
type="text" | ||
value={slippage} | ||
onChange={(e) => handleSlippageChange(e.target.value)} | ||
value={lifecycleStatus.statusData.maxSlippage} | ||
onChange={handleSlippageChange} | ||
disabled={slippageSetting === SLIPPAGE_SETTINGS.AUTO} | ||
className={cn( | ||
color.foreground, | ||
|
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.
Removed the
getMaxSlippage
function.useCallback
is not providing many performance benefits here. Likely it's even slower than accessinglifecycleStatus.statusData.maxSlippage
directlyThere 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.
Nice