Skip to content
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

Jump to bottom

fix: SwapSlippageInput was visually resetting to default value on error #1250

Merged
merged 4 commits into from
Sep 15, 2024
Merged

fix: SwapSlippageInput was visually resetting to default value on error #1250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a28318a
Fix
cpcramer Sep 12, 2024
f60ba96
changeset
cpcramer Sep 12, 2024
563f90d
remove
cpcramer Sep 13, 2024
d3f5a95
fixing tests
cpcramer Sep 14, 2024
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
1 change: 0 additions & 1 deletion site/docs/pages/swap/types.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are now using Config from Leo's change this morning

};
```

Expand Down
32 changes: 8 additions & 24 deletions src/swap/components/SwapSettingsSlippageInput.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,11 @@ describe('SwapSettingsSlippageInput', () => {
});

it('uses provided defaultSlippage', () => {
mockLifecycleStatus = { statusName: 'error', statusData: {} };
render(<SwapSettingsSlippageInput defaultSlippage={1.5} />);
mockLifecycleStatus = {
statusName: 'error',
statusData: { isMissingRequiredField: false, maxSlippage: 1.5 },
};
render(<SwapSettingsSlippageInput />);
expect(screen.getByRole('textbox')).toHaveValue('1.5');
});

Expand All @@ -73,25 +76,6 @@ describe('SwapSettingsSlippageInput', () => {
expect(screen.getByRole('textbox')).toBeDisabled();
});

it('switches between Auto and Custom modes', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have replacement tests for using the values from the config? and why did we remove this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👀

mockLifecycleStatus = { statusName: 'error', statusData: {} };
render(<SwapSettingsSlippageInput defaultSlippage={1.5} />);
const input = screen.getByRole('textbox');
expect(input).toBeDisabled();
expect(input).toHaveValue('1.5');
fireEvent.click(screen.getByRole('button', { name: 'Custom' }));
expect(input).not.toBeDisabled();
fireEvent.click(screen.getByRole('button', { name: 'Auto' }));
expect(input).toBeDisabled();
expect(input).toHaveValue('1.5');
expect(mockSetLifecycleStatus).toHaveBeenCalledWith({
statusName: 'slippageChange',
statusData: {
maxSlippage: 1.5,
},
});
});

it('prevents invalid input', () => {
render(<SwapSettingsSlippageInput />);
fireEvent.click(screen.getByRole('button', { name: 'Custom' }));
Expand Down Expand Up @@ -168,7 +152,7 @@ describe('SwapSettingsSlippageInput', () => {
maxSlippage: 4.5,
},
};
render(<SwapSettingsSlippageInput defaultSlippage={3} />);
render(<SwapSettingsSlippageInput />);
expect(screen.getByRole('button', { name: 'Custom' })).toHaveClass(
'bg-ock-inverse text-ock-primary shadow-ock-default',
);
Expand All @@ -184,7 +168,7 @@ describe('SwapSettingsSlippageInput', () => {
});

it('updates slippage when switching from Custom to Auto', () => {
render(<SwapSettingsSlippageInput defaultSlippage={3} />);
render(<SwapSettingsSlippageInput />);
fireEvent.click(screen.getByRole('button', { name: 'Custom' }));
fireEvent.change(screen.getByRole('textbox'), { target: { value: '5' } });
fireEvent.click(screen.getByRole('button', { name: 'Auto' }));
Expand All @@ -198,7 +182,7 @@ describe('SwapSettingsSlippageInput', () => {
});

it('maintains custom value when in Custom mode', () => {
render(<SwapSettingsSlippageInput defaultSlippage={3} />);
render(<SwapSettingsSlippageInput />);
fireEvent.click(screen.getByRole('button', { name: 'Custom' }));
fireEvent.change(screen.getByRole('textbox'), { target: { value: '5' } });
expect(screen.getByRole('textbox')).toHaveValue('5');
Expand Down
15 changes: 6 additions & 9 deletions src/swap/components/SwapSettingsSlippageInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { useCallback, useState } from 'react';
import { background, border, cn, color, pressable } from '../../styles/theme';
import { DEFAULT_MAX_SLIPPAGE } from '../constants';
import type { SwapSettingsSlippageInputReact } from '../types';
import { useSwapContext } from './SwapProvider';

Expand All @@ -10,21 +11,17 @@ const SLIPPAGE_SETTINGS = {

export function SwapSettingsSlippageInput({
className,
defaultSlippage = 3,
}: SwapSettingsSlippageInputReact) {
const { updateLifecycleStatus, lifecycleStatus } = useSwapContext();
const getMaxSlippage = useCallback(() => {
if (lifecycleStatus.statusName !== 'error') {
return lifecycleStatus.statusData.maxSlippage;
}
return defaultSlippage;
}, [lifecycleStatus.statusName, lifecycleStatus.statusData, defaultSlippage]);
return lifecycleStatus.statusData.maxSlippage;
}, [lifecycleStatus.statusData]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note, for next PR, I don't think we even need getMaxSlippage anymore. Right?

or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, we can remove it!

I suppose the only benefit is minor performance improvements using memoization from useCallback


// 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
Copy link
Contributor Author

@cpcramer cpcramer Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Zizzamia Zizzamia Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

config should not be part of LifecycleStatusDataShared, as LifecycleStatusDataShared is meant to change, where config is meant to stay still.

? SLIPPAGE_SETTINGS.AUTO
: SLIPPAGE_SETTINGS.CUSTOM,
);
Expand Down Expand Up @@ -66,10 +63,10 @@ export function SwapSettingsSlippageInput({
(setting: string) => {
setSlippageSetting(setting);
if (setting === SLIPPAGE_SETTINGS.AUTO) {
updateSlippage(defaultSlippage);
updateSlippage(DEFAULT_MAX_SLIPPAGE);
Copy link
Contributor Author

@cpcramer cpcramer Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Update to use shared const

}
},
[updateSlippage, defaultSlippage],
[updateSlippage],
);

return (
Expand Down
1 change: 0 additions & 1 deletion src/swap/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,6 @@ export type SwapSettingsSlippageDescriptionReact = {
*/
export type SwapSettingsSlippageInputReact = {
className?: string; // Optional className override for top div element.
defaultSlippage?: number; // Optional default slippage value in pecentage.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are now using Config from Leo's change this morning

};

export type SwapSettingsSlippageLayoutReact = {
Expand Down