📊 Single asset page data #310
Conversation
There was a problem hiding this comment.
A couple quick questions. Will leave the next review to @Shadowfiend or @itsrachelfish
ui/components/TabBar/TabBarIcon.tsx
Outdated
| <div> | ||
| <div className={`icon${isActive ? " active" : ""}`} /> | ||
| </div> | ||
| <div className={`icon${isActive ? " active" : ""}`} /> |
There was a problem hiding this comment.
Not a big thing, but can't we use classnames to make this a little cleaner? Maybe it doesn't come up enough in this codebase to warrant it
There was a problem hiding this comment.
I would be fine with this solution as this is plain js and not that hard to understand
as for classnames I had to check the package page, what it does, unerdstand some extra logic for functionality that is mostly not used here
There was a problem hiding this comment.
It's a common React pattern 🤷♂️
I agree this is simple. String interpolation always is until it isn't 😁
There was a problem hiding this comment.
@minutuslausus Just to clarify the decision here: we do use classnames, and it is the correct consistent solution across our codebase for any conditional class name handling. I think this particular piece just predated that decision, and we didn't do a wholesale migration when we added classnames to the mix.
There was a problem hiding this comment.
@Shadowfiend get it now, thanks for the explanation
also I will need to tune my language processor to understand Matt's polite communication style better
| @@ -15,6 +15,7 @@ interface Props { | |||
|
|
|||
| export default function WalletActivityListItem(props: Props): ReactElement { | |||
| const { onClick, activity } = props | |||
| if (!activity.value || !activity.timestamp) return <></> | |||
There was a problem hiding this comment.
Hmm, what's the idea here? What if value is 0 and there's no timestamp because a block couldn't be loaded?
There was a problem hiding this comment.
Unless I'm mistaken, value is a string, so I believe "0" would pass.
I've now tweaked it to check if value is undefined. Granted, in theory, it's redundant given the types. I've also moved the timestamp check to before formatting it, so just the timestamp part is missing when unavailable.
|
What is the purpose of PR reviews?
These are the errors that I am getting. Most of these I are comming from the BigInt issue (I was following the code path for Matt's activity question but faced these errors)
|
|
@henryboldi I have read in one of the commit messages that the asset symbol names should not be used as index. Why does it seem to be a bad idea? Can they change? I am asking this so I understand the contstraints better (also I think it's absolutely fine to implement the filter method the way you did) |
There was a problem hiding this comment.
if the purpose of the PR review is code sanity, nameing etc then I think this is a good PR
the commit history is awesome, and the implementation seems to be appropriate
my only concern is that the code in this branch is not working mainly due to the BigInt issue and probably because it's a bit old. If this not in the scope of the PR review then I am good with this change
|
Thank you for the review, @minutuslausus! By the way, I don't believe any of those errors originate from these changes, but they're important to flag nonetheless!
Primarily, asset symbol names shouldn't be used as IDs because they don't distinguish between different networks. For example, say you have 100 DAI on Polygon and 140 DAI on Mainnet. The UI wouldn't be able to distinguish those using only the token symbol. |
| @@ -12,7 +13,7 @@ export default function TabBarIcon(props: Props): ReactElement { | |||
| return ( | |||
| <> | |||
| <Link to={`/${name}`}> | |||
| <div className={`icon${isActive ? " active" : ""}`} /> | |||
| <div className={classNames("icon", { active: isActive })} /> | |||
There was a problem hiding this comment.
just a quick note that if a new class needs to be added I would put this in a separate variable to make it easier to understand
but usually 2+ is where I draw the line in refactor so I think it's ok like this
Single asset page data
Fills out this single asset page with real data, which opens by clicking on an asset from the home/wallet tab. Currently, this works by filtering the data by the asset symbol (ETH, for example), but this solution will not work once we're multi-network. Having a robust sense of unique IDs would clear this up.