Tokens: Fix hidden you pill on sidebar #1124
Conversation
| @@ -110,13 +111,17 @@ function InfoBoxes({ | |||
| items={stakes} | |||
| renderLegendItem={({ item: account }) => { | |||
| const isCurrentUser = addressesEqual(account, connectedAccount) | |||
| const [label, showLocalIdentityModal] = useIdentity(account) | |||
There was a problem hiding this comment.
We generally leave the "trailing" arguments if they're not used:
| const [label, showLocalIdentityModal] = useIdentity(account) | |
| const [label] = useIdentity(account) |
| /> | ||
| {isCurrentUser && <You />} | ||
| {isCurrentUser && Boolean(label) && <You />} |
There was a problem hiding this comment.
Ah, I wonder if we still need to show this? I understood the change as always showing just the identity badge.
There was a problem hiding this comment.
So, when we have a custom label (like in the first case of the picture) I have to show the custom label or 'YOU'? I think the two things are relevant
I am only replacing for 'YOU' in the badge when it doesnt have a custom label
There was a problem hiding this comment.
Gotcha. I think keeping the "you" makes sense then. Let's double check what happens if you use a long label; ideally we will keep the "you" full width and shrink the label's size.
| return ( | ||
| <div> | ||
| <LocalIdentityBadge | ||
| entity={account} | ||
| connectedAccount={isCurrentUser} | ||
| customLabel={!Boolean(label) && isCurrentUser && 'YOU'} | ||
| labelStyle={(!Boolean(label) && isCurrentUser ? `color: ${theme.tagIndicatorContent};` : '')} |
There was a problem hiding this comment.
I'm wondering—maybe we should move this logic into the LocalIdentityBadge.
We already provide a prop via connectedAccount to control whether or not it's "you", and we can detect and build off of this in that component. We can leave a prop to enable this "special" connected mode, to default the label and style.
There was a problem hiding this comment.
I didnt wanted to do it in LocalIdentityBadge cause I thought this change was only going to be used here because we dont have much space.
There was a problem hiding this comment.
Yes... I was thinking it could be a special mode of the LocalIdentityBadge but this falls apart with needing the label to detect whether we should display the "you".
Could we move to building these props as an object outside? I think it'll be a bit clearer to reason about.
E.g.
const identityBadgeProps = !isCurrentUser || !Boolean(label)
? {}
: {
customLabel: 'YOU'
labelStyle: `color: ${theme.tagIndicatorContent};`
}(Or however prettier prefers 😄)
apps/token-manager/app/src/components/LocalIdentityBadge/LocalIdentityBadge.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@delfipolito I've just added a few commits to polish this up a bit:
- 9e7cd31: simplified the logic for displaying the 'YOU' (since it's always the "default"), and how the widths are adjusted (using flexbox and
flex-shrink: 0) - 1b0bc12: updated the Holders data view entries to dynamically calculate max widths (this may cause some small conflicts with Tokens: Vesting UI #1137)
- 1fb2106, 11d8f98: align
LocalIdentityBadgeimplementations in all the other apps, so they all support thedefaultLabelprop.
This PR closes #1008
With long custom labels it looks like this:
