Ana/emoney-add multidenom rewards to tmbalance & livalidator #3559
Conversation
Bitcoinera
changed the title
|
OK, this is how it is going now. The only thing is that at the end I'm getting the pyramid (below 667px) but I don't know how to do this any way better... I mean, the scrolling how I had it was breaking the styles. Maybe have the pyramid but just make the fonts for rewards smaller? What do you think? Also need to look into the last bit, why is showing that strange margin on the right... GIF:
|
|
Hmm... not working for Terra 🤷♀ So probably not that good. Something is failing. Also still compatibility problems with other networks. I will set it back to "WIP". Not ready yet. |
Bitcoinera
changed the title
|
OK, this branch should be call now "multidenom-format-rewards" 😎 It's also going to include Terra and hopefully all the ones to come Terra:
|
|
OK, now it should be just the test missing and this one is good to go |
| .toString() | ||
| .concat( | ||
| this.isMultiDenomReward | ||
| ? ` ${rewards[0].denom.slice(-3).toUpperCase()}` |
There was a problem hiding this comment.
who is TOM 😉
i don't think this is good logic. we don't need to change the denom.
There was a problem hiding this comment.
hahaha. Yes, I also saw this one. It is outdated though. I am not showing now any denom for single-denom networks. It is not needed.
src/components/common/TmBalance.vue
Outdated
| <h3> | ||
| {{ | ||
| isMultiDenomReward | ||
| ? `Total Rewards in ${stakingDenom}` |
There was a problem hiding this comment.
| ? `Total Rewards in ${stakingDenom}` | |
| ? `${stakingDenom} Rewards` |
Bitcoinera
changed the title
|
I call this one ready! 💪 |
|
OK, maybe it is not so ready. The problem I'm having here is with styles of the positioning of alt-tokens in Desktop view. You can see here if there are only a few it looks kind of weird:
Whereas if there are a lot of them it looks OK:
I am thinking of using some kind of "conditional" CSS |
…responsive fixes and style cleanups
…count (desktop and mobile)
|
Let's merge this amazing PR @Bitcoinera!! not as nice as charlie would make it — but totally great for now!
|
|
OK, this one is definitely ready to go now! Please somebody approve and merge and let's deliver 💪 ! |
…money-format-rewards
src/components/common/TmBalance.vue
Outdated
| @@ -124,6 +158,7 @@ export default { | |||
| ModalTutorial | |||
| }, | |||
| filters: { | |||
| removeUFromMicroDenom, | |||
There was a problem hiding this comment.
this already tells me something is wrong. the UI should not have to handle micro denoms in views
There was a problem hiding this comment.
I thought I had deleted this one... but good that is signaling something wrong 👍
src/components/common/TmBalance.vue
Outdated
| }, | ||
| isMultiDenomReward() { | ||
| if (this.overview.rewards && this.overview.rewards.length > 0) { | ||
| return this.overview.rewards[0].denom !== this.overview.rewards[1].denom |
There was a problem hiding this comment.
are multiple rewards with the same denom possible?
There was a problem hiding this comment.
yes, because of rewards from different validators.
But I don't like this logic either. This is why I want this PR luniehq/lunie-api#343 merged
EDIT: I will add a TODO here
There was a problem hiding this comment.
why does the overview query receive all rewards? doesn't only need a sum per denom?
| rewards: [ | ||
| { | ||
| amount: 1, | ||
| denom: `utoken1` |
There was a problem hiding this comment.
here is the issue. you implemented the API wrong. the rewards need to be delivered in a view token
There was a problem hiding this comment.
Here I fix it: #351
I will change now tests
Closes #ISSUE
Description:
Making the necessary changes to accommodate multidenom rewards.
Please give me all the feedback you can come up with to improve the UI. (Remember that Mario's PR for big numbers will also be added)
The responsiveness is still really bad. It's a work in progress.
Thank you! 🚀
For contributor:
yarn changelogfor a guided process.Files changedin the github PR explorerFor reviewer: