Skip to content

Conversation

@Jon-edge
Copy link
Collaborator

@Jon-edge Jon-edge commented Nov 25, 2023

image

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

#4593

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

@Jon-edge Jon-edge changed the title Jon/ui4/wallet-list UI4: Wallet List Rows Nov 25, 2023
Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

It would be nice to see this PR organized differently, with new components introduced at the beginning, and then integrated into the wallet row at the end. I think that would flow better than tweaking the row step-by-step.

Comment on lines 55 to 60
const showBalance = useSelector(state => state.ui.settings.isAccountBalanceVisible)
const balance = useWalletBalance(wallet, tokenId)
const icon = <CryptoIcon sizeRem={2} tokenId={tokenId} walletId={wallet.id} />
const tickerText = showRate && wallet != null ? <TickerText wallet={wallet} tokenId={tokenId} /> : null
const cryptoText = showBalance ? <CryptoText wallet={wallet} tokenId={tokenId} nativeAmount={nativeAmount ?? balance} withSymbol /> : null
const fiatText = showBalance ? <FiatText nativeCryptoAmount={nativeAmount ?? balance} tokenId={tokenId} wallet={wallet} /> : null
Copy link
Contributor

@swansontec swansontec Nov 27, 2023

Choose a reason for hiding this comment

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

👍 This is really clean & nice. It looks like all your hard work is really starting to pay off, now that we have these nice re-usable components for showing amounts in various ways.

Edit: Oops, I didn't realize this commit was just a copy-paste. Still, it does look nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, this was quite a bit of work 2 years ago. Aiming to get UI4 to the same level of ease of reuse and composability.

Comment on lines 19 to 43
const getPercentDeltaString = (assetToFiatRate: string, assetToYestFiatRate: string, usdToWalletFiatRate: string, theme: Theme) => {
const yesterdayExchangeRate = mul(assetToYestFiatRate, usdToWalletFiatRate)
const yesterdayDelta = sub(assetToFiatRate, yesterdayExchangeRate)

// Avoid divide by zero if there's no exchange rate from yesterday
const yesterdayDeltaPct = zeroString(yesterdayExchangeRate) ? '0' : div(yesterdayDelta, yesterdayExchangeRate, 3)
const yesterdayDeltaPct = zeroString(yesterdayExchangeRate) ? '0' : abs(div(yesterdayDelta, yesterdayExchangeRate, DECIMAL_PRECISION))

// Blank string if yesterday's exchange rate does not exist or delta percent is close enough to 0 (rounding)
if (zeroString(yesterdayExchangeRate) || zeroString(yesterdayDeltaPct)) return { percentString: '', deltaColorStyle: theme.secondaryText }
let percentString
// Prepend a < sign if a nonzero delta rounds to zero
if (!zeroString(yesterdayDeltaPct) && lt(yesterdayDeltaPct, '0.001')) {
percentString = `<${toPercentString({ percentVal: 0.0001, maxPrecision: 2, intlOpts: { noGrouping: true } })}`
} else {
percentString = toPercentString({ percentVal: yesterdayDeltaPct, maxPrecision: 2, intlOpts: { noGrouping: true } })
}

// Colored, signed percentString representing daily price delta. Prepends a '+'
// symbol to the percent string if > 0, otherwise a "-" if < 0.
const percentString = toPercentString(abs(yesterdayDeltaPct), { noGrouping: true })
if (gt(yesterdayDeltaPct, '0')) return { percentString: `+${percentString}`, deltaColorStyle: theme.positiveText }
return { percentString: `-${percentString}`, deltaColorStyle: theme.negativeText }
if (gt(yesterdayDeltaPct, '0')) {
return { percentString: `+${percentString}`, deltaColorStyle: theme.positiveText }
} else if (lt(yesterdayDeltaPct, '0')) {
return { percentString: `-${percentString}`, deltaColorStyle: theme.negativeTextMutedUi4 }
} else {
return { percentString, deltaColorStyle: theme.negativeTextMutedUi4 }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a utility method that isn't exported from the file, it should go at the bottom of the file, below the component body.

Also, it doesn't make sense to copy TickerText -> AssetChangeText, since this component just shows a percent difference, not a fiat rate as the other component does. So we can just introduce the new AssetChangeText component in its own commit, and review it as new code, as opposed to a copy / modify deal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for rebasing the commits - it makes more sense to read now!

However, we still need to move the utility function at the bottom of the file.

@Jon-edge
Copy link
Collaborator Author

Jon-edge commented Nov 27, 2023

It would be nice to see this PR organized differently, with new components introduced at the beginning, and then integrated into the wallet row at the end. I think that would flow better than tweaking the row step-by-step.

Thought there would be value in seeing the first diff for related components to highlight changes, but if not I'll just squash em. I'll see what I can do to rearrange.

@Jon-edge Jon-edge force-pushed the jon/ui4/wallet-list branch 10 times, most recently from 6dc3eed to 5395871 Compare December 5, 2023 01:49
- Copy new Ui4 components to be adjusted
- Integrate SplitRowView into CurrencyRowUi4
@Jon-edge Jon-edge force-pushed the jon/ui4/wallet-list branch from 5395871 to d5e5c26 Compare December 5, 2023 23:17
Based on TickerText.
- Update percent change display according to spec
- Use new colors, add precision, update styling
- Additional update: The rows look weird with a blank percent change for a 0% price difference. Add handling to show <0.01% unless it's truly 0%, in which case a value is still shown.
CryptoIcon + shadow = CryptoIconUi4
@Jon-edge Jon-edge force-pushed the jon/ui4/wallet-list branch 2 times, most recently from ec02c44 to 2bdfcfa Compare December 5, 2023 23:35
@Jon-edge Jon-edge enabled auto-merge December 5, 2023 23:36
@Jon-edge Jon-edge merged commit 8c075f2 into develop Dec 5, 2023
@Jon-edge Jon-edge deleted the jon/ui4/wallet-list branch December 5, 2023 23:47
Jon-edge added a commit that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants