chore: Update SwapSettingsSlippageInput to use Config #1263
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
||
| // 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.
Not needed anymore
| const getMaxSlippage = useCallback(() => { | ||
| return lifecycleStatus.statusData.maxSlippage; | ||
| }, [lifecycleStatus.statusData]); |
There was a problem hiding this comment.
Removed the getMaxSlippage function. useCallback is not providing many performance benefits here. Likely it's even slower than accessing lifecycleStatus.statusData.maxSlippage directly
cpcramer
changed the title
cpcramer
changed the title
cpcramer
changed the title
cpcramer
changed the title
| const [slippageSetting, setSlippageSetting] = useState( | ||
| getMaxSlippage() === DEFAULT_MAX_SLIPPAGE | ||
| lifecycleStatus.statusData.maxSlippage === defaultMaxSlippage |
| // 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.
Well done in updating the comments as well
| @@ -119,7 +113,7 @@ export function SwapSettingsSlippageInput({ | |||
| <input | |||
| id="slippage-input" | |||
| type="text" | |||
| value={slippage} | |||
| value={lifecycleStatus.statusData.maxSlippage} | |||
| onChange={(e) => handleSlippageChange(e.target.value)} | |||
There was a problem hiding this comment.
if you onChange={handleSlippageChange} and handle the event in your callback react won't have to create a new function here on every render
There was a problem hiding this comment.
Very cool! Curious if we prefer that optimization over (in my opinion) code simplicity
Before:
onChange={(e) => handleSlippageChange(e.target.value)}
const handleSlippageChange = useCallback(
(newSlippage: string) => {
const parsedSlippage = Number.parseFloat(newSlippage);
const isValidNumber = !Number.isNaN(parsedSlippage);
// Update slippage to parsed value if valid, otherwise set to 0
updateSlippage(isValidNumber ? parsedSlippage : 0);
},
[updateSlippage],
);
After:
onChange={handleSlippageChange}
const handleSlippageChange = useCallback(
(e: React.ChangeEvent<HTMLInputElement>) => {
const newSlippage = e.target.value;
const parsedSlippage = Number.parseFloat(newSlippage);
const isValidNumber = !Number.isNaN(parsedSlippage);
// Update slippage to parsed value if valid, otherwise set to 0
updateSlippage(isValidNumber ? parsedSlippage : 0);
},
[updateSlippage],
);
There was a problem hiding this comment.
I think it makes sense for a handleChange callback to accept a changeEvent
| setSlippage(0); | ||
| return; | ||
| } | ||
| const parsedSlippage = Number.parseFloat(newSlippage); |
There was a problem hiding this comment.
parseFloat('2.') = 2 preventing you from entering decimals
There was a problem hiding this comment.
I think this is a fast follow update. I belieeve we will need to update getSwapQuote and buildSwapTransaction as well - this is where number <> string conversation happens again
There was a problem hiding this comment.
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.
@cpcramer in your update to getSwapQuote and buildSwapTransaction - can you prevent the APIs from calling when amount=0? seems to be an edge case fired off from the component occasionally
| ? SLIPPAGE_SETTINGS.AUTO | ||
| : SLIPPAGE_SETTINGS.CUSTOM, | ||
| ); | ||
|
|
||
| const updateSlippage = useCallback( | ||
| (newSlippage: number) => { | ||
| setSlippage(newSlippage); | ||
| updateLifecycleStatus({ | ||
| statusName: 'slippageChange', | ||
| statusData: { |
There was a problem hiding this comment.
only call slippageChange if newSlippage !== lifecycleStatus.statusData.maxSlippage
cpcramer
force-pushed
the
paul/clean-swap-settings-slippage-input
branch
from
acdcaf7
to
913c79b
Compare
What changed? Why?
Add the Swap Config to the Context.
Update
SwapSettingsSlippageInputupdates:defaultMaxSlippagerather than our hardcoded maxSlippage value.getMaxSlippagefunction.useCallbackis not providing many performance benefits here. Likely it's even slower than accessinglifecycleStatus.statusData.maxSlippagedirectly.Notes to reviewers
How has it been tested?
