feat(LLM): multibuy 1.5 [LIVE-1710] #90
Conversation
fix: navigation to exchange buy and sell fix swap modal onClose bug fix payment provider name case fix: flow errors and migrate NftImage to ts fix: Exchange select account page not updating after adding an account
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 49cea26 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
mobile
Screenshots: ✅
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
|
|
||
| export default function GooglePay({ height = 12, width = 32 }: Props) { | ||
| return ( | ||
| <Svg viewBox="0 0 32 12" width={width} height={height}> |
There was a problem hiding this comment.
Svg can be complex for overall performance. We should aim to do one of these:
- keep using svg but memo that component
- use an asset instead. static image instead of component.
There was a problem hiding this comment.
Btw, is there a reason we don't use https://react-svgr.com/ ?
With it we can generate components with memo from svg files and it supports RN
avoids conflicts with others and simplify the review
There was a problem hiding this comment.
👍 good overall
my general feedback is that there are way too much unnecessary ifs that are being done. Something feels wrong, do we validate the "route" that gets in? We shouldn't allow our app to have branching everywhere if we have mandatory route passed in our screens. we should be instead validating them on the "navigate to" side.
| accountId: account.id, | ||
| // mode: "buy", | ||
| parentId: parentAccount && parentAccount.id, | ||
| defaultCurrencyId: currency && currency.id, |
There was a problem hiding this comment.
correct me if i'm wrong but it looks like we use
const currency = getAccountCurrency(account);above and according to that function, currency CAN NOT be falsy.
that said according to props, account can be falsy, in that case, I think getAccountCurrency will crash, so there is something weird to fix somewhere: either make sure the account is never falsy, or make a condition on the getAccountCurrency too 🤔
| params: { | ||
| defaultTicker: | ||
| currency && | ||
| currency.ticker && |
| const fiatCurrency = useSelector(counterValueCurrencySelector); | ||
|
|
||
| const filteredProviders = filterRampCatalogEntries(rampCatalog.value[type], { | ||
| cryptoCurrencies: currency.id ? [currency.id] : undefined, |
There was a problem hiding this comment.
both tokencurrency and cryptocurrency have an .id so this if can be simplified
| trade={trade} | ||
| /> | ||
| <WebPlatformPlayer | ||
| onClose={() => {}} |
There was a problem hiding this comment.
no need to pass a empty callback?
|
|
||
| const accounts = useSelector(accountsSelector); | ||
|
|
||
| const availableAccounts = useMemo( | ||
| () => (currency ? getAccountTuplesForCurrency(currency, accounts) : []), |
There was a problem hiding this comment.
according to the route, it's not possible for currency to be falsy
❓ Context
live-mobile]Old PR: Live-1710: Multibuy 1.5 ledger-live-mobile#2340
Implemented Multibuy-1.5 flow rebased on v3
✅ Checklist
📸 Demo
Screen.Recording.2022-05-11.at.15.55.35.mov
🚀 Expectations to reach
Please make sure you follow these Important Steps.
Pull Requests must pass the CI and be internally validated in order to be merged.