-
Notifications
You must be signed in to change notification settings - Fork 301
feat: Remove cryptocurrency-icons #3137
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
1d0a417
to
f711012
Compare
f711012
to
02f1720
Compare
assetClass={props.feed.product.asset_type} | ||
symbol={props.feed.symbol} | ||
/> | ||
<PriceFeedIcon assetClass={props.feed.product.asset_type} /> |
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.
nit: I see <PriceFeedIcon assetClass={props.feed.product.asset_type} />
in a lot of places. We could consider keeping things DRY and hoisting this up to a variable, since it seems it's most the same implementation everywhere:
const priceFeedIcon = <PriceFeedIcon assetClass={props.feed.product.asset_type} />
// then, later, down below
icon: priceFeedIcon
This way, if we need to change this component for whatever reason, we only need to change it in one place 🙂
Summary
In this PR we remove the
cryptocurrency-icons
package completely, so to only use the more generic asset-specific icons.Rationale
There have been asks to update some of the currency icons, but it is difficult for us to keep them all up to date, which is why we'd rather use the generic icons instead.
How has this been tested?