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

[DDW-304]: Implement logarithmic stake slider #2162

Merged
merged 9 commits into from
Sep 9, 2020
Merged

[DDW-304]: Implement logarithmic stake slider #2162

merged 9 commits into from
Sep 9, 2020

Conversation

topseniors
Copy link
Contributor

This PR implemented logarithmic stake slider.
Jira Ticket

Todos

  • Make stake pools ranking slider logarithmic

Testing Checklist

App

  • Go to Staking => Stake Pools and make sure stake pools ranking slider is logarithmic

Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Merge the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

@topseniors topseniors self-assigned this Aug 19, 2020
@ManusMcCole
Copy link

ManusMcCole commented Aug 20, 2020 •
edited
Loading

Hi @yakovkaravelov On Mac Shellley Testnet from hovering over the number 7 it makes it seem as though 7ada is now the minimum amount required to stake.
Screenshot 2020-08-20 at 09 36 06

@ManusMcCole
Copy link

The numbers on the slider are 7, 20, 55, 148 , 403, 1097, 2981..ect. Is there a reason it starts at 7 and not 10 ? If started at 10 the increments can be even numbers which would look a lot tidier. Also it would make more sense as minimum amount to delegate is 10ADA

@ManusMcCole
Copy link

The circulating supply of ada is 25Billion according to coinmarketcap. it is listed as 33Billion on the hover tool tip. Was this figure based on future projections ?
Screenshot 2020-08-20 at 10 11 26
Screenshot 2020-08-20 at 10 12 47

@miorsufianiohk
Copy link

miorsufianiohk commented Aug 20, 2020 •
edited
Loading

@yakovkaravelov I intend to delegate 40,000 ADA but I could only choose either 22,026 or 59,874. My concern is the message "Use the slider to rank the stake pools based on the amount you intend to delegate" is not 100% correct because I am not able to choose 40,000 using the slider.

From user point of view, either the slider needs to be fixed so user is able to choose the specific amount that he wants or the message needs to be altered to explain to the user that he could select an approximation which not necessarily be close to his intended amount.

Annotation 2020-08-20 102137

@topseniors
Copy link
Contributor Author

This needs some quotes from @nikolaglumac and @darko-mijic
Let's solve this once they return.

@topseniors topseniors removed the WIP label Sep 2, 2020
@topseniors
Copy link
Contributor Author

topseniors commented Sep 2, 2020 •
edited
Loading

@gnpf @mioriohk @ManusMcCole
This was updated.
Please review.
Thanks.

NOTE: You can't select other values than slider configured values.

@nikolaglumac
Copy link
Contributor

@ManusMcCole @gnpf please test this one again 🙏

@darko-mijic I would like to hear your opinion too!

@ManusMcCole
Copy link

Hi @yakovkaravelov. Is the the total circulating supply listed not getting changed to 26Billion ? Was there a discussion on this ? Coinmarketcap says almost 26billion is current circulating supply.
Screenshot 2020-09-02 at 16 38 36
Screenshot 2020-09-02 at 16 39 34

@topseniors
Copy link
Contributor Author

Hi @yakovkaravelov. Is the the total circulating supply listed not getting changed to 26Billion ? Was there a discussion on this ? Coinmarketcap says almost 26billion is current circulating supply.
Screenshot 2020-09-02 at 16 38 36
Screenshot 2020-09-02 at 16 39 34

@ManusMcCole Those values were what I got from @nikolaglumac and @darko-mijic

@gabriela-ponce
Copy link

@yakovkaravelov Is there a way to round up some of the small numbers of the sequence? 55, 148, 403 look out of place to me.

@topseniors
Copy link
Contributor Author

topseniors commented Sep 2, 2020 •
edited
Loading

@yakovkaravelov Is there a way to round up some of the small numbers of the sequence? 55, 148, 403 look out of place to me.

@gnpf
I was asked to implement small numbers as they are while big numbers are rounded up.

@ManusMcCole
Copy link

Hi @nikolaglumac. How do you feel about the numbers 55, 148, 403 that @gnpf Gabriela mentioned above ?

@ManusMcCole
Copy link

I see Coingecko says circulating supply is 31billion.If this is the correct figure should somebody email coinmarketcap and tell them to change it ?

@nikolaglumac
Copy link
Contributor

Hi @nikolaglumac. How do you feel about the numbers 55, 148, 403 that @gnpf Gabriela mentioned above ?

@ManusMcCole that one should be signed off by @darko-mijic

@nikolaglumac nikolaglumac self-requested a review September 7, 2020 10:52
nikolaglumac
nikolaglumac suggested changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

@yakovkaravelov can we please make the slider more fluid?
We should also show the values from the "Select a wallet" dropdown "as-is" and not rounded...

@topseniors topseniors removed the WIP label Sep 8, 2020
@topseniors
Copy link
Contributor Author

@nikolaglumac Fixed things. Please review again. Thanks.

@nikolaglumac
Copy link
Contributor

@yakovkaravelov the slider now works great!
There is just one issue - when you use the dropdown to select one of your wallets (or all wallets option) the Api breaks as it looks this sends the amount of stake in a wrong format:

Screenshot 2020-09-08 at 10 25 53

@ManusMcCole
Copy link

Hi @yakovkaravelov. Sorry ignore my previous comment I was on the wrong build

@nikolaglumac
Copy link
Contributor

@ManusMcCole slider works great now!

slider

@ManusMcCole
Copy link

Yeah looks really good now @nikolaglumac . Good work @yakovkaravelov 👍

gabriela-ponce
gabriela-ponce previously approved these changes Sep 8, 2020
View reviewed changes
Copy link

@gabriela-ponce gabriela-ponce left a comment

Choose a reason for hiding this comment

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

Good job 🎉

@nikolaglumac
Copy link
Contributor

@yakovkaravelov all issues fixed in 6f72ee4

@nikolaglumac
Copy link
Contributor

@gnpf @ManusMcCole @yakovkaravelov shouldn't the default slider value be set to "all wallets"?
Can you please compare to the production version?

@topseniors
Copy link
Contributor Author

@gnpf @ManusMcCole @yakovkaravelov shouldn't the default slider value be set to "all wallets"?
Can you please compare to the production version?

@nikolaglumac Default value was set by Darko.

@nikolaglumac nikolaglumac self-requested a review September 9, 2020 09:25
nikolaglumac
nikolaglumac approved these changes Sep 9, 2020
View reviewed changes
Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Excellent work @yakovkaravelov 🎉

@nikolaglumac nikolaglumac merged commit 678fab1 into develop Sep 9, 2020
@iohk-bors iohk-bors bot deleted the feature/ddw-304-logarithmic-stake-slider branch September 9, 2020 09:26
@nikolaglumac nikolaglumac added release-2.3.0-FC1 Daedalus Flight and removed ⏳release-vNext labels Oct 1, 2020
@nikolaglumac nikolaglumac mentioned this pull request Oct 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikolaglumac nikolaglumac nikolaglumac approved these changes

@gabriela-ponce gabriela-ponce gabriela-ponce left review comments

@thedanheller thedanheller Awaiting requested review from thedanheller

@DeeJayElly DeeJayElly Awaiting requested review from DeeJayElly

@darko-mijic darko-mijic Awaiting requested review from darko-mijic

Assignees

@topseniors topseniors

Labels
feature release-2.3.0-FC1 Daedalus Flight
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@topseniors @ManusMcCole @miorsufianiohk @nikolaglumac @gabriela-ponce