Swaps UI update#19169
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
There was a problem hiding this comment.
Most of the logic here is reused from the Build Quote page: https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/swaps/build-quote/build-quote.js
There was a problem hiding this comment.
Most of the logic here is reused from the View Quote page: https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/swaps/view-quote/view-quote.js
There was a problem hiding this comment.
Most of the code here is reused from this component: https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/swaps/dropdown-search-list/dropdown-search-list.js
There was a problem hiding this comment.
Most of the code here is reused from the SlippageButtons component: https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/swaps/slippage-buttons/slippage-buttons.js
|
Awesome work @dan437 ! I have a few comments: 1. Adding tokens simply because we requested a swap quoteI think this is current behavior but adding a token to the user's token list simply because they requested a quote seems like unnecessary clutter 2. Swap details spacingThese controls and text all look really scrunched together -- could use more standardized spacing, maybe from design system? 3. Settings spacingThese controls and text all look really scrunched together -- could use more standardized spacing, maybe from design system? 4. Error off the mapI'm seeing this in other places, especially onboarding, so no idea why this is stretching: 5. MaxI feel like we should disable the "MAX" button if the value is 6. AccessibilityI don't think I could get to these dropdowns via keyboard |
There was a problem hiding this comment.
Can this be added as a color property to Box?
ui/pages/swaps/index.scss
Outdated
There was a problem hiding this comment.
Can color and padding-left be added as box props?
There was a problem hiding this comment.
@dan437 does this TODO need to be resolved before we merge? Or can it be addressed in a future release?
There was a problem hiding this comment.
After merge is fine, because we only redirect to this page if there is QUOTES_EXPIRED_ERROR: https://github.com/MetaMask/metamask-extension/pull/19169/files#diff-8b6b5a47eeec0494313f7bfc546bce1f3e0fbb16bb56d1092884e376ce394095R691
app/scripts/controllers/swaps.js
Outdated
There was a problem hiding this comment.
Why was the swapsFeatureFlags property explicitly added here? This would always initialize the feature flag state to what it was during the last user session. Is that intended?
There was a problem hiding this comment.
Glad you asked! The reason for this is the following: if we would turn off the feature flag for this new UI redesign (in case there is a blocking issue), we would anyway show the new redesign by default before redirecting to the previous design once we get a response from the /featureFlags endpoint, which is not user-friendly. This way we will remember which feature flags were used last time and use those by default, before we get fresh ones from the /featureFlags endpoint.
There was a problem hiding this comment.
Did you check the edge case we mentioned on slack, where the user selects a token to swap that they have not yet added to their token list? The possible failure, given that this PR removes the destinationTokenAddedForSwap and associated logic, is that token balances are not calculated correctly on this page for destination tokens that are not in the users list.
There was a problem hiding this comment.
Yes. It seems that tokensWithBalances is only used for token from in this component and destinationTokenAddedForSwap was for token to. We still call the addToken function from Swaps for token from if that token has a balance, so it should work: https://github.com/MetaMask/metamask-extension/pull/19169/files#diff-2493772b1f68622e8d2eae0660b50d2765884331c60418c86e5315389fb2fd8eR693
|
Hey @dan437, can you debounce fetching the code when the user is typing? Right now it seems it makes the requests to get the quotes just as soon as the user starts typing which feels sluggish and with some lag if the user keeps typing after a couple of characters. Also in the case the user keeps adding numbers, are you cancelling previous requests or how is this handled? Just wondering about an scenario where the user inputs 1 and then 0 (trying to swap 10), and for some reason the request to get a quote for 1, is slower than the request for quotes for 10. So you get a quote to swap 1 instead of 10 |
|
Hey @dan437 , does the new redesign consider this scenario? Right now swaps on full screen looks a bit weird since the layout goes to the bigger size but swaps remains the same.
This also looks like a bug right now:
|
|
Hi @nikoferro, thanks a lot for this valuable feedback. We briefly discussed it internally and here are my responses:
|
There was a problem hiding this comment.
Should this be defaulted to '' since it's not required @georgewrmarshall ?
ui/ducks/swaps/swaps.js
Outdated
There was a problem hiding this comment.
Maybe Boolean(payload) instead?
There was a problem hiding this comment.
Is this eslint line necessary?
There was a problem hiding this comment.
It only silences a warning, but code works without it as well, so I've removed it.
When I pass the handleSearch dependency to remove the warning, it triggers this effect many times. That's why I didn't add it into the dependency list. Maybe there is a better way where the handleSearch fn doesn't trigger this effect.
There was a problem hiding this comment.
Can these first values be assigned at the time of let, and then replaced with if ! priceSlippageUnknownFiatValue.
There was a problem hiding this comment.
Do we want use Text instead of span here @georgewrmarshall
There was a problem hiding this comment.
This PR only adds data-testid attributes in this existing file. We have a user story to refactor a few remaining components in Swaps to use the new DS components (we use them in most of the redesigned Swaps already), so I would prefer do it in another PR.
|
Seeing this weird state where I can submit a swap that I don't have a balance for, for roughly 1 second before I'm told I don't have enough ETH to pay gas fee: MaybeCommit.mov |
|
Can we keep the token list within the bounds of the popup viewport? 
|
Yes, I discussed it with our designers a few weeks ago and there is a ticket for adding a new design system component called |
…l if it's not the only notification
Codecov Report
@@ Coverage Diff @@
## develop #19169 +/- ##
===========================================
- Coverage 70.49% 70.44% -0.05%
===========================================
Files 972 984 +12
Lines 37277 38270 +993
Branches 9636 10004 +368
===========================================
+ Hits 26276 26957 +681
- Misses 11001 11313 +312
|
davibroc
left a comment
There was a problem hiding this comment.
I have completed the tests for this and found no issues
Explanation
We've redesigned our Swaps feature to be easier and faster to use.
There are still a few todos (see below), but we can start getting feedback on code and functionality now to move faster. Most of existing business logic was reused from current Swaps. We keep the old design as a fallback solution, which is controlled via a feature flag. Once we verify that the new redesign is performing well, we will remove the old design from the codebase to make it cleaner.
TODOs:
Screenshots
Manual Testing Steps