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.

Sign up for GitHub

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

Tune displaying account names #1687

Merged
merged 2 commits into from
Jan 15, 2025
+13 −11
Merged

Tune displaying account names #1687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22f6930
Rename prop for clarity
csillag Jan 14, 2025
021c9cd
Re-enable showing account name in account details
csillag Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .changelog/1687.trivial.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fine-tune when to display account names
11 changes: 8 additions & 3 deletions src/app/components/Account/AccountLink.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,12 @@ const WithTypographyAndLink: FC<{
}

interface Props {
showAddressAsName?: boolean
/**
* Should only show the address, never a name
*
* (Use this in situations when the name is already on the screen for some reason.)
*/
showOnlyAddress?: boolean
scope: SearchScope
address: string

Expand Down Expand Up @@ -80,7 +85,7 @@ interface Props {
}

export const AccountLink: FC<Props> = ({
showAddressAsName,
showOnlyAddress,
scope,
address,
alwaysTrim,
Expand All @@ -95,7 +100,7 @@ export const AccountLink: FC<Props> = ({
// isError, // Use this to indicate that we have failed to load the name for this account
} = useAccountMetadata(scope, address)
const accountName = accountMetadata?.name // TODO: we should also use the description
const showAccountName = !showAddressAsName && accountName
const showAccountName = !showOnlyAddress && !!accountName
const to = RouteUtils.getAccountRoute(scope, address)

const extraTooltipWithIcon = extraTooltip ? (
Expand Down
2 changes: 1 addition & 1 deletion src/app/components/Account/RuntimeAccountDetailsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export const RuntimeAccountDetailsView: FC<RuntimeAccountDetailsViewProps> = ({
</StyledListTitleWithAvatar>
<dd>
<AccountLink
showAddressAsName
csillag marked this conversation as resolved.
Show resolved Hide resolved
showOnlyAddress={!!token?.name}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't !!contract more appropriate here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the rationale for hiding the name for all contracts?

(Please note that we can also have other named contracts, not just token contracts. Tokens contracts are already handled.)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah we show other row with name only for token so I think we should use the same condition in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name field is mandatory for ERC-20 tokens, but not for other tokens.
That is why the name field (of the token data structure) is defined like this:

  /** Name of the token, as provided by token contract's `name()` method. */
  name?: string;

So it's possible that we have tokens without names. So displaying a token link doesn't necessarily mean that we have a valid name there. That's why I used token.name for the condition.

This way, if the token doesn't have a name, but the smart contract does, we can still display it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok if it's optional than it make sense to keep as is 👍

scope={account}
address={address!}
highlightedPartOfName={highlightedPartOfName}
Expand Down
2 changes: 1 addition & 1 deletion src/app/components/Tokens/TokenDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export const TokenDetails: FC<{
<dd>
<span>
<AccountLink
showAddressAsName
showOnlyAddress
scope={token}
address={token.eth_contract_addr ?? token.contract_addr}
/>
Expand Down
2 changes: 1 addition & 1 deletion src/app/components/Tokens/TokenList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export const TokenList = (props: TokensProps) => {
content: (
<span>
<AccountLink
showAddressAsName
showOnlyAddress
scope={token}
address={token.eth_contract_addr ?? token.contract_addr}
alwaysTrim
Expand Down
6 changes: 1 addition & 5 deletions src/app/pages/TokenDashboardPage/TokenDetailsCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,7 @@ export const TokenDetailsCard: FC<{ scope: SearchScope; address: string; searchT
)}
<dt>{t(isMobile ? 'common.smartContract_short' : 'common.smartContract')}</dt>
<dd>
<AccountLink
showAddressAsName
scope={account}
address={account.address_eth || account.address}
/>
<AccountLink showOnlyAddress scope={account} address={account.address_eth || account.address} />
<CopyToClipboard value={account.address_eth || account.address} />
</dd>

Expand Down
Loading