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

Chore: Swap Refactors #1733

Closed
wants to merge 3 commits into from
Closed

Chore: Swap Refactors #1733

wants to merge 3 commits into from

Conversation

brendan-defi
Copy link
Contributor

What changed? Why?

  • refactored swap to prep for wallet island

Notes to reviewers

How has it been tested?
locally

Copy link

vercel bot commented Dec 13, 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 Dec 17, 2024 3:32pm
onchainkit-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 17, 2024 3:32pm
onchainkit-routes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 17, 2024 3:32pm

@@ -75,6 +76,7 @@ export function Swap({
data-testid="ockSwap_Container"
>
<div className="mb-4 flex items-center justify-between">
{backButton}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the backButton for again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need a backbutton in walletisland's implementation of swap
image

Copy link
Contributor

Choose a reason for hiding this comment

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

To me this feels like something that should live outside of Swap. I don't think that we want to ship that option to third party consumers of the component.

Could it instead be a button that's positioned absolutely to the parent? I think that'd make more sense than adding a backButton prop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the issue is how to center the title between the backbutton and the settings button.

the parent container for these is set to justify-between. this is the correct default behavior. i have not figured out a way to override this from "within" the container — that is, no class directive on the swap-title makes it center-aligned.

AFAICT, i need a third element so that the title will move to the center.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best that I can come up with is to make the topbar relative, wrap swapSettings in a div, and make that div absolutely-positioned with right-0.

@brendan-defi
Copy link
Contributor Author

closing in favor of a cleaner branch

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.

2 participants