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

Fix/4379 member acount balance #4450

Merged
merged 20 commits into from
Nov 20, 2023

Conversation

chrlschwb
Copy link
Contributor

fix #4379

@vercel
Copy link

vercel bot commented Jun 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
dao ✅ Ready (Inspect) Visit Preview Nov 16, 2023 1:22pm
pioneer-2 ✅ Ready (Inspect) Visit Preview Nov 16, 2023 1:22pm
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Nov 16, 2023 1:22pm

Copy link
Collaborator

@traumschule traumschule left a comment

Choose a reason for hiding this comment

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

@chrlschwb
| [warn] src/common/components/BlockTime/BlockHistoryLine.tsx
| [warn] src/memberships/components/MemberProfile/MemberAccounts.tsx
| [warn] Code style issues found in 2 files. Forgot to run Prettier?
| yarn lint failed with exit code 1

@thesan wrt storybook:

Error: No existing credentials found. Please run vercel login or pass "--token"

{!!member.rootAccount && (
<AccountMemberRow>
<UnknownAccountInfo address={member.rootAccount} placeholderName="Root Account" />
<TokenValue value={rootBalance?.total} isLoading={!isDefined(rootBalance?.total)} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

What could be the reason that it keeps blinking (never is defined)?

blinking

@chrlschwb
Copy link
Contributor Author

Uploading image.png…
I have error when run yarn lint:fix

@chrlschwb
Copy link
Contributor Author

image

I have error when run yarn lint:fix

@chrlschwb chrlschwb requested a review from freakstatic October 20, 2023 01:05
Copy link
Collaborator

@kdembler kdembler left a comment

Choose a reason for hiding this comment

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

Left few comments. Also on visuals, I think the amount should be right-aligned, currently different values will be misaligned:
CleanShot 2023-11-13 at 10 28 08@2x

onClick={() => setSelectedWallet(wallet)}
selected={selectedWallet?.extensionName === wallet.extensionName}
/>
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fragment doesn't seem needed?


const AccountsDisplay = styled(RowGapBlock)`
padding: 24px;
`

export const AccountMemberRow = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to duplicate this style? We can use AccountRow as was imported before, and overwrite necessary CSS with styled(AccountRow)

Comment on lines 19 to 23
{!!account && (
<AccountMemberRow>
<UnknownAccountInfo address={account} placeholderName="Root Account" />
<TokenValue value={balance?.total} isLoading={!isDefined(balance?.total)} />
</AccountMemberRow>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic id duplicated 3 times: 1 one for root, 1 for controller and once in this component. I think we should adjust this component to be more generic and also use it for root and controller accounts

@kdembler
Copy link
Collaborator

Hi @chrlschwb my last comment wasn't resolved. What I was suggesting was to make MemberBounds into something like this:

export const MemberAccount = ({ account, name }: Props) => {
  const balance = useBalance(account)
  return (
    <div>
      {!!account && (
        <AccountMemberRow>
          <UnknownAccountInfo address={account} placeholderName={name} />
          <TokenValue value={balance?.total} isLoading={!isDefined(balance?.total)} />
        </AccountMemberRow>
      )}
    </div>
  )
}

Then you can use it for all accounts, like root:

      {!!member.rootAccount && (
        <MemberAccount account={member.rootAccount} name="Root account" />
      )}

This way you can also move style next to this component, removing circular dependency between two files

@chrlschwb
Copy link
Contributor Author

chrlschwb commented Nov 15, 2023

yeah, I understand.

Copy link
Collaborator

@kdembler kdembler left a comment

Choose a reason for hiding this comment

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

Could you just update the MemberBounds component and file name now that it's generic? Otherwise, LGTM

@chrlschwb
Copy link
Contributor Author

what do you think the name?

@chrlschwb
Copy link
Contributor Author

Could you just update the MemberBounds component and file name now that it's generic? Otherwise, LGTM ?

what?

@kdembler
Copy link
Collaborator

I suggested MemberAccount earlier

Copy link
Collaborator

@thesan thesan left a comment

Choose a reason for hiding this comment

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

Nice work, it looks great 👍

@thesan thesan merged commit 5e0385d into Joystream:dev Nov 20, 2023
3 checks passed
@ivanturlakov
Copy link

Tested on https://pioneer-2.vercel.app/#/profile/memberships
Mainnet

✅ Balance added
Screenshot 2023-11-23 at 19 35 36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-dev issue suitable for community-dev pipeline qa-tested-ready-for-prod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show balance on Accounts tab
5 participants