-
Notifications
You must be signed in to change notification settings - Fork 207
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
Add token icons #55
Add token icons #55
Conversation
Add services/known_tokens. Update store/actions accordingly. Update common/constants accordingly.
Add common token svg files (taken from 0x instant).
Conflicts: src/util/types.ts
Another thing, we could have some kind of fallback icon as seen in the designs (screenshot below): a component that renders the first 3 letters of the token's symbol with a coloured-circle background as in the mocks. For now, |
I like the idea of a fallback icon, and it shouldn't be too hard to do. @unjapones can you open an issue for that? |
I agree with @fvictorio, fallback icons are a good Idea and should be considered in a new issue |
|
||
const getTokenIconNameBySymbol = (symbol: string): string => { | ||
const symbolfirstChar = symbol.charAt(0).toUpperCase(); | ||
const restOfSymbol = symbol.substring(1, symbol.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can drop the second argument here, the API is similar to that of Array.prototype.slice
.
|
||
export const TokenIcon = (props: Props) => { | ||
const { token } = props; | ||
const TokenIconComponentName = getTokenIconNameBySymbol(token.symbol) as keyof typeof TokenIcons; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keyof typeof TokenIcons
mind = blown
src/services/known_tokens.ts
Outdated
} from '../util/token_meta_data'; | ||
import { Token } from '../util/types'; | ||
|
||
export class KnownTokens { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts on this.
First, can we move this to the util
directory, based on what we talk about having pure/testable things there?
Second, it would be nice to make KNOWN_TOKENS_META_DATA
a constructor parameter, so that this can be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup agree with both items. Will work on that now.
src/services/known_tokens.ts
Outdated
|
||
export class KnownTokens { | ||
private readonly _tokens: Token[] = []; | ||
private readonly _wethToken: Token | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something here, but I think there's no need to make this nullable. And the app shouldn't work without the weth token anyway.
Conflicts: src/common/constants.ts src/store/actions.ts
Add token icons
Connects #11.
/src/common
,/src/services
and/src/util
).TokenIcon
component to render an SVG, given a token symbol.I might add some notes in the code for clarification.