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

feat: polish balance page #1370

Merged
merged 16 commits into from
Apr 22, 2024
Merged

feat: polish balance page #1370

merged 16 commits into from
Apr 22, 2024

Conversation

panteleymonchuk
Copy link
Collaborator

@panteleymonchuk panteleymonchuk commented Mar 28, 2024

Description of change

Closes #1352

Type of change

  • Bug fix (a non-breaking change which fixes an issue)

How the change has been tested

Take a look address page.

Desktop
image

Tablet
image

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

  • My code follows the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Signed-off-by: Eugene Panteleymonchuk <[email protected]>
@begonaalvarezd begonaalvarezd marked this pull request as draft April 1, 2024 15:33
@panteleymonchuk panteleymonchuk linked an issue Apr 2, 2024 that may be closed by this pull request
@panteleymonchuk panteleymonchuk marked this pull request as ready for review April 18, 2024 09:30
Copy link
Collaborator

@brancoder brancoder left a comment

Choose a reason for hiding this comment

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

LGTM 🎊

Copy link
Collaborator

@brancoder brancoder left a comment

Choose a reason for hiding this comment

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

Should we remove the empty Conditionally locked mana panel?

image

@begonaalvarezd @panteleymonchuk wdyt?

Copy link
Collaborator

@brancoder brancoder left a comment

Choose a reason for hiding this comment

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

Resolve conflicts please 🙏

@panteleymonchuk
Copy link
Collaborator Author

Should we remove the empty Conditionally locked mana panel?

image

@begonaalvarezd @panteleymonchuk wdyt?

it makes sense to me

# Conflicts:
#	client/src/app/components/nova/address/AddressBalance.tsx
@panteleymonchuk
Copy link
Collaborator Author

@brancoder
According to logic that was before I covered 2 more cases. Don't show blockIssuanceCredits or manaRewards if value `null.

blockIssuanceCredits !== null ? manaFactory(blockIssuanceCredits, "Block issuance credits:", formatManaBalanceFull, setFormatManaBalanceFull)
                            : null,

@brancoder
Copy link
Collaborator

@panteleymonchuk There's something weird with storage deposit on this address rms1qrmc0qhqadnsm0nefxunnhtexsrgqr6wa3aest640q4a4t0lcruj5ff7wa7
image

@panteleymonchuk
Copy link
Collaborator Author

hmm, seems I even don't see data.
Maybe I have wrong config or so 🤔
image

@panteleymonchuk panteleymonchuk marked this pull request as draft April 19, 2024 14:41
Signed-off-by: Eugene Panteleymonchuk <[email protected]>
@panteleymonchuk panteleymonchuk marked this pull request as ready for review April 19, 2024 15:17
Copy link
Member

@begonaalvarezd begonaalvarezd left a comment

Choose a reason for hiding this comment

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

Looking so much better than before 💪🏼 great job
Just couple of notes:

  • I would add a top level mana value so that its easy to read how much mana you have instead of needing to sum manually
    image

  • The BIC is not displaying right
    image

  • If the balance is 0, we should display it
    image
    image

Copy link
Member

@begonaalvarezd begonaalvarezd left a comment

Choose a reason for hiding this comment

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

2 extra small details

  • We should unify showing always unit when 0 or not so we avoid these situations
    image
  • The copy feature could be improved so you always copy the "raw amount", the syll amount in the subunit unit, without the ticker, so you copy the full value number, and not a string, eg if the balance is 1.123 TST the value copied should be 1123000 instead of 1.123 TST

@begonaalvarezd begonaalvarezd self-requested a review April 22, 2024 07:28
Copy link
Member

@begonaalvarezd begonaalvarezd left a comment

Choose a reason for hiding this comment

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

As commented in slack, lets detatch the unit toggles from different boxes

Signed-off-by: Eugene Panteleymonchuk <[email protected]>
Copy link
Collaborator

@brancoder brancoder left a comment

Choose a reason for hiding this comment

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

LGTM

@brancoder brancoder merged commit a0bd434 into dev Apr 22, 2024
6 checks passed
@brancoder brancoder deleted the feat/issues-1352-polish-balance branch April 22, 2024 14:35
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.

[Task]: Polish balance section in address page
3 participants