-
Notifications
You must be signed in to change notification settings - Fork 15
fix: change user overview info #3332
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
base: master
Are you sure you want to change the base?
Conversation
rdig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jakubcolony
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR @adam-strzelec 👍
Unfortunately, when I voted support as a user who did not stake, I got the failed message despite the motion passing:

| actionData.motionData.motionStateHistory.hasFailedNotFinalizable; | ||
|
|
||
| const userStake = usersStakes.find(({ address }) => address === userAddress); | ||
| const userVote = voterRecord.some((item) => item.address === userAddress); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about hasUserVoted to indicate it's a boolean and not the vote value?
| ? 'motion.finalizeStep.failed.statusText' | ||
| : 'motion.finalizeStep.statusText', | ||
| id: | ||
| isMotionFailedNotFinalizable || (!userStake && userVote) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I'm following this. Why do we want to show failed status text if user did not stake but has voted?
|
@jakubcolony done. According to Arren, in the case where "user who did not stake but did vote" and with the outcome passed, it should have the description "Action has passed and can be executed." |
jakubcolony
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm still unsure this is correct. When I staked and voted, I saw this when motion was ready to be finalized:
Shouldn't that say "Action has passed and can be executed"? Before finalization, I don't think we can consider the action to be "complete".
Then as a user who staked but not voted, the same motion step had a different label:

And that persisted even past finalization (at which point the action has been executed, so the label seems incorrect):

Do you actually need to take into account whether the user has voted? I would've thought you only need to consider these cases:
- Motion ready to be finalized: Action has passed and can be executed.
- Motion finalized: Action has passed and is now complete.
- Motion failed: Keep existing copy.
@adam-strzelec Just to confirm, Jakub is correct here. Everyone should see the same description in the last steps. |
bassgeta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iamsamgibbs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working great for motions which are approved:
User who staked: (It does show a negative reward which I don't think is correct).
User who did not (should a user who did not stake but has a reward because they voted, be able to claim the reward? I think they should but there is no claim button here for them)
After claiming as the other user, the claimed pill shows here.
As a user who did not stake or vote:
However, when a motion is rejected, the text still says it has passed:
And when finalised the rejection:
There is also no claim button but the UserHub implies there should be.
I think the claiming issues are out of scope for this, but the wording should be fixed up here for rejected motions.
|
|
@adam-strzelec we might need some clarification from @arrenv for all of these:
|
1494d33 to
10456e2
Compare
|
iamsamgibbs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job, thanks for the changes.
The wording is now correct for a failed motion:
Also looks good as a user who voted but did not stake:
This issue I mentioned previously with the transaction showing "Claimable" but no claim button being visible is occurring on master, so can definitely be addressed separately.
| actionData, | ||
| refetchColony, | ||
| ]); | ||
| // console.log('action data: ', actionData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover comment
bassgeta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rdig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approving good to go
jakubcolony
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @adam-strzelec, this is good to go. Just a reminder about the leftover console.log.
Failed without staking or voting:

On a side note, there is a bug here (not introduced by this PR): #4004
Also a bug here, again not introduced by this PR. Added to the issue above.
Nice work! 💪
05b8a7c
2c9f434
b3c24ec to
2c9f434
Compare
|
@iamsamgibbs I rebased it 😉 @jakubcolony @mmioana Can you check it one more time? |
iamsamgibbs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmioana
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @adam-strzelec! Thank you a lot for your efforts! 🙌
|
Hey @adam-strzelec! This needs a rebase |
|
@mmioana I'm working on that right now 🙂 |
c089b8e
2c9f434 to
c089b8e
Compare
mmioana
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adam-strzelec tested your PR and still looks good 👍 Nice work!🥇
iamsamgibbs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…py for opposed action
a14c212
c089b8e to
a14c212
Compare
|
@bassgeta @mmioana @iamsamgibbs I had to resolve conflicts. Can you check it one more time? |












































Description
Change appearance of action overview card info


Testing
Diffs
New stuff ✨
Display date of complete action
Changes 🏗
Change text inside cards
Resolves #31415