Skip to content
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

TokenBadge Component #221 #334

Merged
merged 17 commits into from May 13, 2019

Conversation

Projects
None yet
4 participants
@rperez89
Copy link
Contributor

commented Mar 5, 2019

@bpierre @sohkai Hey guys is not ready to be merged i have some questions about how the icon, how are we going to get it? and probably we need a new AddressField component right? because this one is using the EthIdentIcon.

Screen Shot 2019-03-27 at 4 17 05 PM

Screen Shot 2019-03-27 at 4 18 03 PM

@luisivan luisivan referenced this pull request Mar 22, 2019

Closed

Review aragonUI PRs #38

3 of 3 tasks complete
@AquiGorka

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Hey @rperez89 could you post a couple of screenshots of this new component in the description?

@rperez89

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Hey @rperez89 could you post a couple of screenshots of this new component in the description?

Yes, sure! it's not finished as i did some questions on how to handle the icon. But is pretty similar to the IdentityBadge, as you will see on the popup i am not showing the right icon because i wanted to know if i should do a new AddressField component in order to display the new icon

@sohkai sohkai requested a review from bpierre Mar 27, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

On the icon, we should expect the user to input it into this component. However, @bpierre, I'm not sure if there's a way to make it generic between an img src and an svg component?

For token icons I imagine most people would like to use an URI.

@bpierre bpierre added this to the @aragon/ui@1.0.0 milestone Apr 26, 2019

@bpierre

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@rperez89 Sorry to have been so long to review this PR, it’s looking good! I added a commit to sync it with the master branch.

However, @bpierre, I'm not sure if there's a way to make it generic between an img src and an svg component?

Yes, we could use <img /> by default (for external SVG / PNG images), and maybe also have another prop to let users set their own component if they want an inline SVG or something else?

@rperez89 @sohkai I wonder if we shouldn’t let people pass a provider function to resolve tokens in general, rather than having to use an extended version of this component in their apps? That way, we could also use it from other components in the future. We could still keep the props on the badge itself, so that some specific values could still be set on a single badge easily.

import { TokensProvider } from '@aragon/ui'
import trustedTokens from './trusted-tokens'

// This function could also be async
function resolveToken(address) {

  const token = trustedTokens.get(address)

  if (!token) {
    return null
  }

  return {
    name: token.name,
    symbol: token.symbol,
    imageUrl: token.imageUrl,
  }
}

function MyApp() {
  return (
    <TokensProvider resolver={resolveToken}>
      <div>
        <TokenBadge address="0xcafe" />
      </div>
    </TokensProvider>
  )
}

We could also let users access it with a Hook:

import { useTokenResolver } from '@aragon/ui'

function TokenInfo({ address }) {

  const { name, symbol, imageUrl, loading, exists } = useTokenResolver(address)

  if (!exists) {
    return 'The token couldn’t be found.'
  }

  if (loading) {
    return 'Loading…'
  }

  return (
    <ul>
      <li>Image: <img src={imageUrl} alt="" /></li>
      <li>Name: {name}</li>
      <li>Symbol: {symbol}</li>
    </ul>
  )
}

In the future, we could also use this mechanism to provide a default list of trusted tokens from aragon/aragon, either from @aragon/api-react (by declaring <TokensProvider>) or from @aragon/ui (by pulling the data from @aragon/api-react then declaring <TokensProvider>), so that token badges would just work ® without having to do anything. App authors would then have the possibility to extend or replace this default provider.

Show resolved Hide resolved devbox/apps/TokenBadge.js Outdated
Show resolved Hide resolved devbox/apps/TokenBadge.js Outdated
Show resolved Hide resolved src/components/TokenBadge/TokenBadge.js Outdated
@sohkai

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

I wonder if we shouldn’t let people pass a provider function to resolve tokens in general, rather than having to use an extended version of this component in their apps?

Ohh yes, I think this would be great 👍!

In the future, we could also use this mechanism to provide a default list of trusted tokens from aragon/aragon

Yes, or we could have an API that allows the app to check if the token is trusted by the Aragon client (e.g. app.verifyToken(<addr>). Would also make it a bit easier to work with an IPFS metadata registry in the future. aragon/aragon.js#293

@bpierre bpierre referenced this pull request May 2, 2019

Open

Add TokensProvider #364

bpierre added some commits May 8, 2019

Add a useImageExists hook
And a corresponding ImageExists component (render prop).
TokenBadge: display token icons + other changes
- Rename `entity` to `address` (since it is always an address in that case)
- Make `symbol` a required prop.
- Remove `shorten` (only used by IdentityBadge).
- Remove `fontSize` (there should be no variations in font size).
- Display the icon if it exists (badge + popover).
- Add a label to the popover close button.
@bpierre

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@rperez89 @sohkai I added a few more commits! This idea is to get a first version of the badge released without the tokens provider, then implement it as a second step.

Changes:

  • Add a useImageExists / ImageExists utility.
  • AddressField: allow the icon to be replaced.
  • Add a tokenIconUrl() utility (get a token icon URL from the contract address).
  • TokenBadge:
    • Rename entity to address (since it is always an address in that case)
    • Make symbol a required prop.
    • Remove shorten (only used by IdentityBadge).
    • Remove fontSize (there should be no variations in font size).
    • Display the icon if it exists (badge + popover).
    • Add a label to the popover close button.

@bpierre bpierre requested a review from sohkai May 10, 2019


return (
<React.Fragment>
<ButtonBase
ref={this._element}
title={address}
onClick={address && this.handleOpen}
title={`${label}${address || 'No address'}`}

This comment has been minimized.

Copy link
@sohkai

sohkai May 10, 2019

Member

Should this also use the isValidAddress check?

color: ${theme.textPrimary};
`}
>
<Main title={`${name} (${symbol})`}>
<Label>
<Icon src={`https://chasing-coins.com/coin/logo/${symbol}`} />

This comment has been minimized.

Copy link
@sohkai

sohkai May 10, 2019

Member

Something we may want to consider is using this as a backup on networks other than mainnet (so the app still shows token icons on rinkeby)

{label}
</h1>
{iconUrl ? (
<ImageExists src={iconUrl}>

This comment has been minimized.

Copy link
@sohkai

sohkai May 10, 2019

Member

Wonder if it makes sense to bake this url check into ImageExists itself, so you can safely use this component at no performance cost if the src is falsey

This comment has been minimized.

Copy link
@bpierre

bpierre May 11, 2019

Member

Yes totally, it doesn’t hurt 👍

@sohkai

sohkai approved these changes May 10, 2019

Copy link
Member

left a comment

😍 This is so nice!

bpierre added some commits May 11, 2019

@bpierre bpierre requested a review from sohkai May 11, 2019

}

// TODO: ensure only one image is loading at a time for a given src.

This comment has been minimized.

Copy link
@sohkai

sohkai May 13, 2019

Member

No need to do this now, but there's a super simple util for doing this in aragon.js that works by caching an async function's result: AsyncRequestCache

This comment has been minimized.

Copy link
@bpierre

bpierre May 13, 2019

Member

Oh nice yes!

const cache = new Map()

// Delete the first (oldest) entry until we are back to `size`,
// using a loop because getValue() is async.

This comment has been minimized.

Copy link
@sohkai

sohkai May 13, 2019

Member

I don't think this part of the comment applies? cache.keys().next() should be synchronous.

This comment has been minimized.

Copy link
@bpierre

bpierre May 13, 2019

Member

Yes you’re right! The Map only gets updated after getValue() returned so it’s all sync at this point and cache should never be more than 101. It might or might not have been useful at some point during the implementation 😄

Show resolved Hide resolved src/utils/cached-map.js Outdated
@sohkai

sohkai approved these changes May 13, 2019

Copy link
Member

left a comment

❤️ the caching component (really nice way to make an LRU cache!), just a few small comments and this is good to go!

@bpierre

This comment has been minimized.

Copy link
Member

commented May 13, 2019

LRU cache

TIL 👀

cachedMap(): remove fetch()
The caching module itself doesn’t need to be async. It could be
implemented in another module, alongside request deduplication.

@bpierre bpierre force-pushed the rperez89:TokenBadge branch from c5610e5 to 4d28bbc May 13, 2019

@bpierre bpierre merged commit 0edc827 into aragon:master May 13, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.