fix: SwapSlippageInput was visually resetting to default value on error #1250
Conversation
github-actions
bot
added
documentation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
cpcramer
changed the title
| @@ -169,7 +169,6 @@ type SwapSettingsSlippageDescriptionReact = { | |||
| ```ts | |||
| type SwapSettingsSlippageInputReact = { | |||
| className?: string; // Optional className override for top div element. | |||
| defaultSlippage?: number; // Optional default slippage value in pecentage. | |||
There was a problem hiding this comment.
We are now using Config from Leo's change this morning
| if (lifeCycleStatus.statusName !== 'error') { | ||
| return lifeCycleStatus.statusData.maxSlippage; | ||
| } | ||
| return defaultSlippage; | ||
| }, [lifeCycleStatus.statusName, lifeCycleStatus.statusData, defaultSlippage]); |
There was a problem hiding this comment.
No need for all these checks post lifecycleStatusShared update.
| @@ -67,10 +64,10 @@ export function SwapSettingsSlippageInput({ | |||
| (setting: string) => { | |||
| setSlippageSetting(setting); | |||
| if (setting === SLIPPAGE_SETTINGS.AUTO) { | |||
| updateSlippage(defaultSlippage); | |||
| updateSlippage(DEFAULT_MAX_SLIPPAGE); | |||
There was a problem hiding this comment.
Update to use shared const
cpcramer
force-pushed
the
paul/fix-slipppage-on-error
branch
from
dc3c5b4
to
422ed28
Compare
| @@ -245,7 +245,6 @@ export type SwapSettingsSlippageDescriptionReact = { | |||
| */ | |||
| export type SwapSettingsSlippageInputReact = { | |||
| className?: string; // Optional className override for top div element. | |||
| defaultSlippage?: number; // Optional default slippage value in pecentage. | |||
There was a problem hiding this comment.
We are now using Config from Leo's change this morning
cpcramer
changed the title
| @@ -74,26 +68,6 @@ describe('SwapSettingsSlippageInput', () => { | |||
| expect(screen.getByRole('textbox')).toBeDisabled(); | |||
| }); | |||
|
|
|||
| it('switches between Auto and Custom modes', () => { | |||
There was a problem hiding this comment.
do we have replacement tests for using the values from the config? and why did we remove this test?
There was a problem hiding this comment.
These tests were using the old defaultSlippage input param. I was going to remove and write new ones but we are still getting 100% test coverage 👀
cpcramer
force-pushed
the
paul/fix-slipppage-on-error
branch
from
422ed28
to
3e1c845
Compare
|
|
||
| // Set initial slippage values to match previous selection or default, | ||
| // ensuring consistency when dropdown is reopened | ||
| const [slippage, setSlippage] = useState(getMaxSlippage()); | ||
| const [slippageSetting, setSlippageSetting] = useState( | ||
| getMaxSlippage() === defaultSlippage | ||
| getMaxSlippage() === DEFAULT_MAX_SLIPPAGE |
There was a problem hiding this comment.
This should use config.maxSlippage.
The default max slippage can be customized using the config parameter.
It might be nice to add and track the defaultMaxSlippage in LifecycleStatusDataShared.
type LifecycleStatusDataShared = {
defaultMaxSlippage: number; // set using config.maxSlippage and does not change.
isMissingRequiredField: boolean;
maxSlippage: number; // set using config.maxSlippage but updates when slippage is customized. This will remain what slippage value we use.
};
Thoughts? @0xAlec
There was a problem hiding this comment.
config should not be part of LifecycleStatusDataShared, as LifecycleStatusDataShared is meant to change, where config is meant to stay still.
cpcramer
force-pushed
the
paul/fix-slipppage-on-error
branch
from
54186fe
to
563f90d
Compare
| return defaultSlippage; | ||
| }, [lifecycleStatus.statusName, lifecycleStatus.statusData, defaultSlippage]); | ||
| return lifecycleStatus.statusData.maxSlippage; | ||
| }, [lifecycleStatus.statusData]); |
There was a problem hiding this comment.
Side note, for next PR, I don't think we even need getMaxSlippage anymore. Right?
or am I missing something?
There was a problem hiding this comment.
Good point, we can remove it!
I suppose the only benefit is minor performance improvements using memoization from useCallback
What changed? Why?
Notes to reviewers
How has it been tested?