Change Crypto UI #269
Change Crypto UI #269
Conversation
There was a problem hiding this comment.
This looks awesome @Reza9472 Although there is a regression in terms of number of coins the UI looks much better.
I left a few comments, give them a look.
| const response = await fetch( | ||
| "https://data.messari.io/api/v1/assets?fields=id,name,symbol,metrics/market_data/price_usd,metrics/market_data/ohlcv_last_24_hour", | ||
| `https://data.messari.io/api/v1/assets/btc/metrics/price/time-series`, |
There was a problem hiding this comment.
A user would only be able to bitcoin prices now.
There was a problem hiding this comment.
Yes I only implemented bitcoin for now to get the ok from you for the UI.
| ); | ||
|
|
||
| export const options = { | ||
| responsive: true, |
There was a problem hiding this comment.
Although the chart is surely responsive, it's not doing much help for mobile users as can be seen below. What do you think?
There was a problem hiding this comment.
hmmm, that's interesting. Thought it would keep the format for mobile. Gotta go through the chart.js to see if they have any options for this
| <Element> | ||
| Name :<strong>{values.name}</strong> | ||
| </Element> | ||
| <img src="https://img.icons8.com/color/24/000000/bitcoin--v1.png" /> |
There was a problem hiding this comment.
Thought about adding another library that had the icons but seemed like overkill since messari doesn't have a lot of cryptos
| High :<strong>{values.values.at(-1)[2].toFixed(2)}</strong> | ||
| </Element> | ||
| <Element> | ||
| Low :<strong>{values.values.at(-1)[3].toFixed(2)}</strong> |
There was a problem hiding this comment.
Not a crypto trader myself but maybe we shouldn't use toFixed here, a lot of times bigger coins like bitcoin vary after 2 decimal places. What do you think?
There was a problem hiding this comment.
I checked googles crypto and most were 2 decimals. but you are right some of them do. I'll check the ones messari has and change it. But still think It's cleaner to have the toFixed but maybe with more decimals
|
@sahil-shubham can you resolve the conflicts and merge PR if it looks good? |
|
@hargup This can't be merged right now as there is a regression in terms of number of coins offered by the app. I can work on it and merge it then. |
|
It has been more than a month, we need to close this PR within next few days either ways. Depending on how many coins are missing and how important they are, you have two options:
|
|
Closing this PR for now as its a decision between just bitcoin with chart or all the coins without a chart. Feel free to re-open this if interested in adding it. Thank you for putting in the time for this, it would be helpful to anyone working on this issue in the future. |
Fixes #242
Hi please review and tell me if there are any changes you want to the design