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

Tokens: Fix hidden you pill on sidebar #1124

Merged
merged 8 commits into from
Jun 9, 2020
Merged

Tokens: Fix hidden you pill on sidebar #1124

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9ddac9c
Modify local identity badge for current user
delfipolito Apr 9, 2020
ca94d1d
Old style when badge has custom label
delfipolito Apr 10, 2020
cf91af0
Build props outside component
delfipolito Apr 13, 2020
9e7cd31
Tokens: update logic for showing 'YOU' in InfoBox
sohkai Jun 9, 2020
1b0bc12
Tokens: update compact widths for holders table
sohkai Jun 9, 2020
1fb2106
Apps: align LocalIdentityBadge implementations
sohkai Jun 9, 2020
11d8f98
Agent: fix linting
sohkai Jun 9, 2020
035ed9f
Tokens: fix IdentityBadges in InfoBox defaulting to 'YOU'
sohkai Jun 9, 2020
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
7 changes: 6 additions & 1 deletion apps/token-manager/app/src/components/InfoBoxes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { formatBalance, stakesPercentages } from '../utils'
import { addressesEqual } from '../web3-utils'
import LocalIdentityBadge from './LocalIdentityBadge/LocalIdentityBadge'
import You from './You'
import { useIdentity } from './IdentityManager/IdentityManager'

const DISTRIBUTION_ITEMS_MAX = 7

Expand Down Expand Up @@ -110,13 +111,17 @@ function InfoBoxes({
items={stakes}
renderLegendItem={({ item: account }) => {
const isCurrentUser = addressesEqual(account, connectedAccount)
const [label, showLocalIdentityModal] = useIdentity(account)
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally leave the "trailing" arguments if they're not used:

Suggested change
const [label, showLocalIdentityModal] = useIdentity(account)
const [label] = useIdentity(account)


return (
<div>
<LocalIdentityBadge
entity={account}
connectedAccount={isCurrentUser}
customLabel={!Boolean(label) && isCurrentUser && 'YOU'}
labelStyle={(!Boolean(label) && isCurrentUser ? `color: ${theme.tagIndicatorContent};` : '')}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Contributor

@sohkai sohkai Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 😄)

/>
{isCurrentUser && <You />}
{isCurrentUser && Boolean(label) && <You />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I wonder if we still need to show this? I understood the change as always showing just the identity badge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

</div>
)
}}
Expand Down
3 changes: 2 additions & 1 deletion apps/token-manager/app/src/components/LocalIdentityBadge/LocalIdentityBadge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ const LocalIdentityBadge = ({ entity, ...props }) => {
const network = useNetwork()
const [label, showLocalIdentityModal] = useIdentity(entity)
const handleClick = () => showLocalIdentityModal(entity)

return (
<IdentityBadge
label={label || ''}
label={label || props.customLabel }
sohkai marked this conversation as resolved.
Show resolved Hide resolved
entity={entity}
networkType={network && network.type}
popoverAction={{
Expand Down