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

Jump to bottom

Balance Module: Update latest movement message when not effective #355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Balance Module: Update latest movement message when not effective #355

wants to merge 1 commit into from

Conversation

fabriziovigevani
Copy link
Contributor

@fabriziovigevani fabriziovigevani commented Jun 3, 2020 •
edited
Loading

fixes #340

Shows a Effective next term message when the movement is immediate and not yet effective.

By immediate we mean that the movement had an immediate impact on the total balance.

E.g for activating tokens, the active balance is already increased, the increased amount is not yet effective in that is not going to be taken into account for drafting.

Was not totally sure of using the sandglass icon as suggested by @john-light #340 (comment) since it wouldn't give you an idea of whether the movement is positive or not. For example, another immediate movement that can be not yet effective is slashing.

Preview

Not effective
Screen Shot 2020-06-02 at 9 56 07 PM

Effective

Screen Shot 2020-06-02 at 9 59 31 PM

@vercel
Copy link

vercel bot commented Jun 3, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/aragon/court-dashboard/evggngb04
✅ Preview: https://court-dashboard-git-latest-movement-fix.aragon.now.sh

sohkai
sohkai reviewed Jun 3, 2020
View reviewed changes
src/components/Dashboard/Balance.js
{convertToString(activity.type, activity.direction)}
{activity.isImmediate && !activity.isEffective
? 'Effective next term'
: convertToString(activity.type, activity.direction)}
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 if we should have a tooltip for the Active part of the balance module when it's Effective next term.

I can still see a user being confused about their "immediate" active balance vs. their "effective" active balance at the moment, as we only show one value in the module.

src/components/Dashboard/Balance.js
@@ -218,7 +218,9 @@ const LatestActivity = ({ activity, distribution }) => {
color: ${theme.content};
`}
>
{convertToString(activity.type, activity.direction)}
{activity.isImmediate && !activity.isEffective
? 'Effective next term'
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we should have different text for the Inactive part of the balance module to clarify where the inactive portion has moved (to be withdrawal or activated).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm yeah perhaps doesn't make sense to show the effective next term message there since is not something that is relevant for that balance and we can just return the Activated.

@john-light
Copy link
Contributor

  1. In the "Not effective" screenshot, where it says +9490 ANJ Effective next term is this 9490 ANJ included in the balance of Active: 9990 ANJ shown directly above? Or will the 9490 ANJ only be added to the Active balance in the next term? (and then the Active balance will be 19480 ANJ)

  2. In the "Effective" screenshot, under the "Inactive" column it says -9490 ANJ Activated. Should it say "Activated" here? Or Deactivated? And should it show a - sign or a + sign, if the token balance is getting added to this column? (Similar question for the -10,000 ANJ Withdrawal shown on the "My Wallet" column of this screenshot.)

@fabriziovigevani
Copy link
Contributor Author

In the "Not effective" screenshot, where it says +9490 ANJ Effective next term is this 9490 ANJ included in the balance of Active: 9990 ANJ shown directly above? Or will the 9490 ANJ only be added to the Active balance in the next term? (and then the Active balance will be 19480 ANJ)

It is included already in the balance yes.

In the "Effective" screenshot, under the "Inactive" column it says -9490 ANJ Activated. Should it say "Activated" here? Or Deactivated? And should it show a - sign or a + sign, if the token balance is getting added to this column? (Similar question for the -10,000 ANJ Withdrawal shown on the "My Wallet" column of this screenshot.)

The - a + signs reflects how the latest movement affected that balance. In the case of the inactive balance it shows the - because the 9490 ANJ went from the inactive to the active balance.

Regarding the message, the intention of it is to show the reason of why the balance increased/decreased and in this the inactive balance decreased by 9490 ANJ because it was activated.

Do you see that a little confusing?

Regarding the wallet balance movement, in our local environment we run a script which does two txs to initiate balances, first moves the amount from the wallet balance to the inactive balance(and that's why it's showing the withdrawal) before moving it to the active balance. (In production this would not occur) since the dashboard only allows moving from wallet balance to active balance in one tx.
But in the rare case where someone would create txs outside the dashboard and does a staking tx, then the amount would move from the wallet balance to the inactive balance and that's the movement you would see.

@john-light
Copy link
Contributor

It is included already in the balance yes.

I think the ANJ should only be included in the active balance once it is "effective".

The - and + signs reflects how the latest movement affected that balance. In the case of the inactive balance it shows the - because the 9490 ANJ went from the inactive to the active balance.

Regarding the message, the intention of it is to show the reason of why the balance increased/decreased and in this the inactive balance decreased by 9490 ANJ because it was activated.

Do you see that a little confusing?

It is a bit confusing but perhaps only because I lacked context for why the ANJ was being deducted from the Inactive column. We can leave this part as-is for now.

@sohkai sohkai changed the base branch from development to master July 3, 2020 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sohkai sohkai sohkai left review comments

@john-light john-light Awaiting requested review from john-light

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Balance is shown as active even though it isn't counted as active in the court
3 participants
@fabriziovigevani @john-light @sohkai