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

Display type in tokens table #685

Merged
merged 5 commits into from Jul 9, 2023
Merged

Conversation

csillag
Copy link
Contributor

@csillag csillag commented Jul 6, 2023

Now that we have two types of tokens, ERC-20 and ERC-721, it helps to show which is which in the tables.

Before After
Token list horizontal l
image image
Token list vertical
image image
Token dashboard
image image
Account details
image image

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Deployed to Cloudflare Pages

Latest commit: 3985b5b8cd17edabb2c006e507df37947653c759
Status:✅ Deploy successful!
Preview URL: https://b4c06e19.oasis-explorer.pages.dev

@csillag csillag force-pushed the csillag/show-type-in-token-table branch from ec39378 to b5842b4 Compare July 6, 2023 17:51
@csillag csillag marked this pull request as ready for review July 6, 2023 17:51
@csillag
Copy link
Contributor Author

csillag commented Jul 6, 2023

@donouwens what do you think?

@csillag csillag requested review from buberdds and lukaw3d July 6, 2023 19:08
.changelog/681.feature.md Outdated Show resolved Hide resolved
@csillag csillag force-pushed the csillag/show-type-in-token-table branch from b5842b4 to 61597e7 Compare July 6, 2023 20:20
@csillag
Copy link
Contributor Author

csillag commented Jul 7, 2023

We could also consider using some kind of icons instead of written text, if there were good icons for this. (Are there?)

@donouwens
Copy link
Collaborator

I'm very happy with how it looks already @csillag

I agree that icons would be nice, but I've struggled to come up with anything that's clearly identifiable at a glance (erc20 vs erc721), so I'd opt for having tags like this (not set on the colors yet but obviously each type would have its own color). Would that be okay for you?

@csillag
Copy link
Contributor Author

csillag commented Jul 7, 2023

@donouwens To be honest, I am not sure about our nomenclature here. Looking at Etherscan, they simply call the ERC-20 stuff "Tokens" and the ERC-721 and similar stuff "NFTs". (But then for the second category, they do show "type" in the table. Maybe what they are doing is not a bad idea?

But then this brings up the question, do we even want to have these two categories (tokens and NFTs) in the same table? If yes, let's just say "Tokens" and "NFTs" as the type? Or have two different tables? Does this also mean new routes and new pages?

So many possibilities.

In any case, I'm ok with colored tags, but I also would also consider adding "NFT" to the tags like this: NFT (ERC-721)

@lukaw3d
Copy link
Member

lukaw3d commented Jul 7, 2023

My suggestion would be: same table for both, no type column, name = "Wrapped Rose" | "AI ROSE (NFT)". But then vertical view becomes "AI ROSE (AI ROSE) (NFT)"

@donouwens
Copy link
Collaborator

@csillag True, but that leaves us in a pickle further down the line when we add ERC-1155 to the list as these can be NFTs or fungible tokens. I think your final suggestion could work in which we combine the type and "NFT" or "Token", just not only "NFT" or "Token". I've updated the visual now.

We should keep everything combined in one table as we will add filtering options in the next iteration after public launch which should address that point.

@lukaw3d - but again if we add ERC-1155 to that mix we'd need to show more than just NFT or Token, right?

Still working through the colors btw..

@csillag
Copy link
Contributor Author

csillag commented Jul 7, 2023

My suggestion would be: same table for both, no type column, name = "Wrapped Rose" | "AI ROSE (NFT)". But then vertical view becomes "AI ROSE (AI ROSE) (NFT)"

Could the (NFT) added to the name be a tag like this?

image

And can we put the ERC-721 into the tooltip?

@buberdds buberdds self-requested a review July 7, 2023 10:38
@csillag csillag force-pushed the csillag/show-type-in-token-table branch 2 times, most recently from f923562 to 0d579e3 Compare July 7, 2023 13:41
@csillag
Copy link
Contributor Author

csillag commented Jul 7, 2023

Tweaked the look according to the visuals Updated screenshots above.

@csillag csillag requested a review from lukaw3d July 7, 2023 13:55
@donouwens
Copy link
Collaborator

@csillag

  1. Can we try it with the type not being bold, like in the visuals - there is a different font color, but the same weight within the pills.

  2. The pills seem taller than the visuals (compare them to the verified pills - I used the same sizing for both)

  3. Hopefully reducing the height would hopefully make them look better on mobile

@csillag csillag force-pushed the csillag/show-type-in-token-table branch from 0d579e3 to a64cc77 Compare July 7, 2023 15:38
@csillag
Copy link
Contributor Author

csillag commented Jul 7, 2023

Updated styling. I replaced the first 2 screenshot, but not the others.

@csillag csillag force-pushed the csillag/show-type-in-token-table branch 2 times, most recently from 5cfba24 to 624764c Compare July 9, 2023 14:28
@csillag csillag force-pushed the csillag/show-type-in-token-table branch from 624764c to 3985b5b Compare July 9, 2023 21:35
@csillag csillag merged commit 40e64d2 into master Jul 9, 2023
7 checks passed
@csillag csillag deleted the csillag/show-type-in-token-table branch July 9, 2023 22:05
@@ -15,12 +25,40 @@ type TokensProps = {
pagination: false | TablePaginationProps
}

export const TokenTypeTag: FC<{ tokenType: EvmTokenType; sx?: SxProps }> = ({ tokenType, sx = {} }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to TokenPills that wraps around mui's Chip.

We should choose more consistent names for "the background rounded label thing"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about DogTag ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I mean for a common ancestor, with common sizing and fond settings, and then create or descendants from that, with consistent names, either Tag or Pill)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants