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.

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

feat: Add Swap Settings #1210

Merged
merged 23 commits into from
Sep 6, 2024
Merged

feat: Add Swap Settings #1210

merged 23 commits into from
Sep 6, 2024

Conversation

cpcramer
Copy link
Contributor

@cpcramer cpcramer commented Sep 4, 2024

What changed? Why?
Add SwapSettings sub-component - we are not displaying it yet.

Update SwapSettingsSlippageInput

  • Fix a bug to allow the input field to clear.
  • Fix a bug where closing and opening the Slippage dropdown would reset state.

Add maxSlippage field to statusData in every LifeCycleStatus except Error.
Notes to reviewers

How has it been tested?

Copy link

vercel bot commented Sep 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
onchainkit-coverage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 6:15pm
onchainkit-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 6:15pm
onchainkit-routes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 6:15pm

@cpcramer cpcramer requested a review from alessey September 5, 2024 16:38

const [maxSlippage, _setMaxSlippage] = useState(
experimental.maxSlippage || 3,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

don't want this in state do we? If the experimental prop changes, should that update here?

Copy link
Contributor Author

@cpcramer cpcramer Sep 5, 2024

Choose a reason for hiding this comment

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

The experimental prop won't change after the component is rendered.

It will set the initial maxSlippage value. Any updates will be handled by LifeCycleStatus.slippageChange.maxSlippage property

https://github.com/coinbase/onchainkit/pull/1210/files#diff-9bca97e90be8d6cfaa2c6813dbf1d684ed792b80b395f6b033d3d06963f23371R91-R94

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@cpcramer cpcramer Sep 5, 2024

Choose a reason for hiding this comment

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

Some context on why we are setting it this way

Screenshot 2024-09-05 at 11 30 36 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are only using this const to initialize lifecycle status, can it just be a const? or alternatively we could move the initiallifecycle status into a const?

Copy link
Contributor

Choose a reason for hiding this comment

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

The experimental prop won't change after the component is rendered.

@cpcramer that sentence it's incorrect! We should always expect a prop can change, that's the all point of prop. :)

@cpcramer cpcramer requested a review from Zizzamia September 5, 2024 23:30
const maxSlippage =
lifeCycleStatus.statusName !== 'error'
? lifeCycleStatus.statusData.maxSlippage
: 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

If 3 is our default for everything, this should be in a share const.

Copy link
Contributor

Choose a reason for hiding this comment

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

Follow up on this in the next PR.

Copy link
Contributor

@Zizzamia Zizzamia left a comment

Choose a reason for hiding this comment

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

I want to keep the ball moving, so I am going to merge this, and @cpcramer follow up with the notes in your next PR.

@Zizzamia Zizzamia merged commit 5b98428 into main Sep 6, 2024
16 checks passed
@Zizzamia Zizzamia deleted the paul/swap-settings-settings branch September 6, 2024 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants