feat: finish up UI changes and code styling #24429
Conversation
|
CLA Signature Action: Thank you for your submission, we really appreciate it. We ask that you all read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:
By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository. 1 out of 2 committers have signed the CLA. |
BZahory
force-pushed
the
mb843/swap-and-send
branch
from
fbf3ac0
to
a3fa6a7
Compare
ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/pages/send/components/quote-card/hooks/useEthFeeData.tsx
Show resolved
Hide resolved
ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
Show resolved
Hide resolved
| @@ -93,6 +98,7 @@ export function SwappableCurrencyInput({ | |||
| )} | |||
| asset={asset?.details} | |||
| isSkeleton={isAmountLoading} | |||
| isMatchingUpstream={isSetToMax} | |||
There was a problem hiding this comment.
Why not just call const isSetToMax = useSelector(getSendMaxModeState); in CurrencyInput?
There was a problem hiding this comment.
CurrencyInput should be decoupled from Send logic (see: #23192 (comment))
There was a problem hiding this comment.
Gotcha, I think the naming could be more descriptive then - maybe shouldSyncValueWithUpstreamState
There was a problem hiding this comment.
Isn't starting with "is" convention?
There was a problem hiding this comment.
hmm good point - looks like is/has/can is the convention. I'm good with any prefix as long as the name is more descriptive
There was a problem hiding this comment.
Hmm yeah, it's tricky because it's technically always syncing with the upstream; this explicitly says that it will always match the upstream
| // symbol to cover edge case where initial value is literally 'INITIAL_VALUE' | ||
| const INITIAL_VALUE = Symbol('INITIAL_VALUE'); | ||
|
|
||
| export default function useStateWithFirstTouch<T>( |
There was a problem hiding this comment.
It lets us know if the token amount has been modified by a user input in CurrencyInput so we can know if the amount is being updated upstream after opening the send flow via the "EDIT" flow
There was a problem hiding this comment.
gotcha, i feel like there's some way we can use the usePrevious hook within or instead of this hook. no need to look into it if there's other things to fix tho. unless the usage is obvious (i didn't dive too deep)
can this be reused instead of setIsTokenInputChanged in asset-picker-amount or are the behaviors different?
There was a problem hiding this comment.
It's a little different since we are able to reset that one, so a normal state will do there. useStateWithFirstTouch should do for now imo – it's the cleanest solution I can think of
| @@ -22,7 +22,7 @@ export function NFTInput({ integerValue, onChange, className }: NFTInputProps) { | |||
| return; | |||
| } | |||
|
|
|||
| onChange(newValue.toPrefixedHexString()); | |||
| onChange(newValue.toPrefixedHexString(), String(newValueAsString)); | |||
| }; | |||
|
|
|||
| return ( | |||
There was a problem hiding this comment.
Is there a way to make the amount input autofocus? That's the behavior for tokens, but doesn't seem to work for NFTs
There was a problem hiding this comment.
autofocus is super inconsistent so I just added logic to focus on key press if that's okay
There was a problem hiding this comment.
I see it working when I click Send from the NFTs tab in the overview. But not if I go to the NFTs tab from within Send. Low priority tho, so the latest version lgtm
There was a problem hiding this comment.
Hmm I see it working for both
ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
Outdated
Show resolved
Hide resolved
## Explanation ### What is the current state of things and why does it need to change? With MetaMask/metamask-extension#23703, we will be implementing a new type of transaction, swap+send – a swap targeting the recipient. To properly track this transaction, we will need a new transaction type with new properties. ### What is the solution your changes offer and how does it work? We add a new `swapAndSend` transaction type with properties that track the source+destination tokens, as well as the recipient. ### Are there any changes whose purpose might not obvious to those unfamiliar with the domain? We include certain swap properties that we may not use initially (e.g., `swapTokenValue`, `swapMetaData`) in case we add more parity with the swaps functionality over time. ### If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? The `TransactionType` enum was abstracted to this library rather than the Extension. ### If you had to upgrade a dependency, why did you do so? N/A ## References [MB-843](https://consensyssoftware.atlassian.net/browse/METABRIDGE-843) MetaMask/metamask-extension#23703 (MetaMask/metamask-extension#24429 latest in merge train targeting that branch) <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? For example: * --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/transaction-controller` - **ADDED**: `swapAndSend` type and respective - **ADDED**: `updateSwapAndSendTransaction()` - **ADDED**: `TransactionMetaBase` properties: `destinationTokenAmount`, `sourceTokenAddress`, `sourceTokenAmount`, `sourceTokenDecimals`, `swapAndSendRecipient` ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
| @@ -140,6 +140,9 @@ const QUOTE_VALIDATORS = [ | |||
| type: 'number', | |||
| }, | |||
| ]; | |||
|
|
|||
| const SWAP_AND_SEND_SLIPPAGE = '2'; | |||
There was a problem hiding this comment.
just confirming, does swaps have a default slippage and is it the same as this?
There was a problem hiding this comment.
I couldn't find one. I just found this code in review-quote.js:
const [slippageErrorKey] = useState(() => {
const slippage = Number(fetchParams?.slippage);
if (slippage > 0 && slippage <= 1) {
return SLIPPAGE_LOW_ERROR;
} else if (slippage >= 5 && slippage <= MAX_ALLOWED_SLIPPAGE) {
return SLIPPAGE_HIGH_ERROR;
}
return '';
});
This indicates that the range for swaps quotes is 1-5 (both exclusive). I'd imagine we'd want to be on the lower end given the typical use cases for a send, so 2 seems like a clean number to choose.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| &--size-xs { | ||
| --button-icon-size: 20px; | ||
| } |
There was a problem hiding this comment.
Should this be removed? Started a discussion in slack regarding changes in #24150
There was a problem hiding this comment.
Will address both comments in a follow-up PR in the merge train
| @@ -58,7 +58,7 @@ | |||
| text-decoration: none; | |||
| } | |||
|
|
|||
| &--disable-underline:hover:not(&--disabled){ | |||
| &--disable-underline:hover:not(&--disabled) { | |||
There was a problem hiding this comment.
See comment above
| @@ -2804,7 +2806,8 @@ export function signTransaction(history) { | |||
|
|
|||
| await dispatch(actions.setPrevSwapAndSend(prevSwapAndSendData)); | |||
|
|
|||
| if (stage === SEND_STAGES.EDIT) { | |||
| // you can only edit a basic send transaction | |||
| if (stage === SEND_STAGES.EDIT && !isSwapAndSend) { | |||
There was a problem hiding this comment.
Does this mean we can remove the prev swap send state from redux as well?
There was a problem hiding this comment.
In theory but I’d rather keep it for now since we may be reutilizing it p soon. A bit of a boat anchor that we can clear pending future discussion on confirmations
No description provided.