-
Notifications
You must be signed in to change notification settings - Fork 145
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
Store swap asset info in routing #4317
Comments
Skipping this issues while #4425 is in flight so I don't mess up route changes. |
@kyranjamie is this issue still needed? Can you clarify why this is necessary? I can't say I totally understand what I'd be implementing here? So, as the user selects each token the route changes? |
It's not absolutely necessary, but it's good application design. A core principal of the web is that locations have unique identifiers in the form of a URL. When building features, and even designing them, it's helpful to consider how they map to URLs, as this is easier than trying to retrofit functionality later.
I'd suggest the implementation saves the state of which tokens are selected in the URL, rather than custom component/context state.
Yeah, that's how all exchanges work. The URL changes when you go to another trading pair. Examples 👇🏼 Benefits of building features with descriptive URLs:
|
I presume that this improved routing will also help with SEO once we move swaps to the web app. |
These are just centralized exchange pairs, I was looking for a defi swapping UI example. ALEX doesn't do it, but I do see that Jupiter does. 👍 |
Just checked Uniswap does not. 🤔 |
Don't see relevance of centralised or decentralised. Swaps and trades are effectively the same action to a user. There's an exchange of one asset for another, and the interfaces are identical. Unisat is not an example we want to follow 😅 This isn't a discussion specific to swaps, though. It's a question of: Does it make senes for this information to go in the URL? Examples asides, I'm curious to hear your take @fbwoolf. What is the argument against doing this? |
I think you are mistaking my curiosity with 'arguing' against something. I think you are misreading my comments here. I don't have an argument against it. I def think it is better. I was just looking to play around with the experience of a swapping UI where the user (me) was seeing the route change based on the tokens I was selecting (or typing into the url). I just hadn't come across it before, or used. My comment on the centralized exchange was that the pairs are set up to choose from typically where I don't see that with decentralized exchanges ...but I might just be missing something there. |
All good. Sorry, I did indeed read your comments in a way thinking you were counter arguing—which is totally fine if you disagree. Gloves off 🥊 |
I started on this work yest, but realized it is going to conflict quite a bit with changes in the container work with the |
## [6.33.0](v6.32.1...v6.33.0) (2024-04-10) ### Features * add balances shimmer loader, closes [#5119](#5119) ([5c1c284](5c1c284)) * add src-20 token balances, closes [#3751](#3751) ([fb859b6](fb859b6)) * add stacks balance loader ([20418ab](20418ab)) * change query persister to chrome storage, closes [#5153](#5153) ([1cd2625](1cd2625)) * compliance checks ([6df0869](6df0869)) * stacks ft fiat values from alex-sdk, closes [#4653](#4653) ([0f7e44e](0f7e44e)) * support multiple recipients in rpc send transfer method, closes [#5174](#5174) ([a470a57](a470a57)) ### Bug Fixes * add border to onboarding form ([a6bda2d](a6bda2d)) * container when resized ([909fa0c](909fa0c)) * dependabot ([d927ec0](d927ec0)) * deprecate InfoCard to add border correctly ([b6864cd](b6864cd)) * fix routing issues with send flow ([f32151d](f32151d)) * only show messages on homepage ([8228c11](8228c11)) * refetch brc20 tokens on window focus ([a985e0f](a985e0f)) * shimmer styles import ([868ee71](868ee71)) * swap test ([85eb975](85eb975)) * swap toggle with new routing ([f179f3e](f179f3e)) * use signed stacks account in transaction [#4923](#4923) ([6dca269](6dca269)) ### Internal * Add wallet user survey, adjust styling ([3c242cf](3c242cf)) * disable compliance check ([b4b1d11](b4b1d11)) * fmt ([a937795](a937795)) * implement fix to limit amount of accounts rendered ([629ef97](629ef97)) * post-release merge back ([3c9c0f8](3c9c0f8)) * replace drawer dialog, containers and global header footers, onboarding, settings, ref [#4371](#4371) ([6262267](6262267)) * swaps routes, closes [#4317](#4317) ([70c51a1](70c51a1)) * ugprade dev packages ([4ed8326](4ed8326)) * update express, ref [#5130](#5130) ([264bf8d](264bf8d)) * update prettier package ([e75990f](e75990f)) * update stx avatar ([03fe093](03fe093)) * update undici, ref [#4956](#4956) ([8019e0d](8019e0d)) * update webpack + axios, ref [#5090](#5090) ([77803f5](77803f5)) * upgrade redux toolkit, redux ([2eb8090](2eb8090))
Suggest we use a scheme of lowercase urls
The text was updated successfully, but these errors were encountered: