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-554] Show "Loading Stake Pools" anytime the pools are loading #2424

Merged
merged 7 commits into from
Mar 5, 2021
Merged

[DDW-554] Show "Loading Stake Pools" anytime the pools are loading #2424

merged 7 commits into from
Mar 5, 2021

Conversation

thedanheller
Copy link
Contributor

@thedanheller thedanheller commented Mar 2, 2021 •
edited by nikolaglumac
Loading

This PR enables the "Loading Stake Pools" state anytime the pools are loading.

Todos

  • Add the loading state for any occasion where the Stake Pools are loading
  • Increase the checker interval for "fetching data directly" case (or maybe for all the cases)
  • Make sure stake pool tracker watches for 6 consecutive stagnations of stake pools count

Screenshots

Screen Shot 2021-03-02 at 16 55 03
Screen Shot 2021-03-02 at 16 55 14

Testing Checklist

Test Cases

Scenario 1 - Validate Loading on Stake Pools screen after clean installation

Given that Daedalus is installed on a clean state
And it's synced
And stake pools are still loading
When I access to "Stake pools" on the Staking center
Then I can see the text "Loading stake pools" || "ステークプールをロード中"
And a spinner is displayed next to the number of stake pools displayed

Scenario 2 - Validate Loading on Stake Pools screen after switching servers

Given that Daedalus is synced
When I change the SMASH server on the "Settings" screen
And I access to "Stake pools" on the Staking center
Then I can see the text "Loading stake pools" || "ステークプールをロード中"
And a spinner is displayed next to the number of stake pools displayed

Scenario 3 - Validate Loading on Stake Pools screen after switching servers and restart

Given that Daedalus is synced
And the SMASH server on the "Settings" screen was recently changed
When I restart Daedalus
And I access to "Stake pools" on the Staking center
Then I can see the text "Loading stake pools" || "ステークプールをロード中"
And a spinner is displayed next to the number of stake pools displayed

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
  • PR has default-sized Daedalus window screenshots or animated GIFs of important UI changes:
    • In English
    • In Japanese
  • 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

@thedanheller thedanheller self-assigned this Mar 2, 2021
@DeeJayElly DeeJayElly self-requested a review March 3, 2021 08:26
DeeJayElly
DeeJayElly reviewed Mar 3, 2021
View reviewed changes
Copy link
Contributor

@DeeJayElly DeeJayElly left a comment

Choose a reason for hiding this comment

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

@daniloprates I have a feeling we should fix the distance between the label and the spinner as it looks different on English and Japanese.

Screen Shot 2021-03-03 at 2 26 21 AM

Screen Shot 2021-03-03 at 2 26 14 AM

miorsufianiohk
miorsufianiohk approved these changes Mar 3, 2021
View reviewed changes
Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

Functionality working as expected. Great work @daniloprates 👍 . Tested on 16895

@gabriela-ponce
Copy link

@daniloprates Overall, this works fine, but I noticed the spinner goes away after a while, even when the list is still loading (when fetching the data directly).

Check the screenshots: 1, 2, 3, 4.
In all these, the number of stake pools is still increasing, and the loading state is not visible anymore.

@thedanheller
Copy link
Contributor Author

@daniloprates Overall, this works fine, but I noticed the spinner goes away after a while, even when the list is still loading (when fetching the data directly).

Check the screenshots: 1, 2, 3, 4.
In all these, the number of stake pools is still increasing, and the loading state is not visible anymore.

@gabriela-ponce we don't have an exact way of checking if the pools are still loading. We just keep checking if the number is increasing. If the number increases after our checking, there's not much do be done 🤷🏻‍♂️

@nikolaglumac
Copy link
Contributor

@gabriela-ponce @daniloprates I believe we should adjust the checker interval. Especially for the "fetch data directly" case as indeed that one is quite slow when fetching the data...
I will put a TODO to this PR for this change.

@nikolaglumac nikolaglumac self-assigned this Mar 5, 2021
@nikolaglumac nikolaglumac removed the WIP label Mar 5, 2021
@nikolaglumac
Copy link
Contributor

@gabriela-ponce @miorsufianiohk please re-test this one. I have added a fix which should resolve the issue you were seeing with the loading spinner disappearing while the stake pools are still loading.

@miorsufianiohk
Copy link

@nikolaglumac I tested this on testnet which had 253 stake pools in total. During testing, I noticed that when the stake pools number was showing 253, the 'loading stake pools' animation was still going on, it only disappeared after more than 2 minutes. Is this acceptable? My expectation was, when the number was showing 253, that's the end of loading (finished, no animation should be displayed).
Screenshot 2021-03-05 at 10 54 55

@nikolaglumac
Copy link
Contributor

@miorsufianiohk that is indeed correct and expected behavior!
The spinner should hide ~3 mins after the last change in stake pools count.

nikolaglumac
nikolaglumac reviewed Mar 5, 2021
View reviewed changes
source/renderer/app/stores/StakingStore.js
@@ -239,6 +241,8 @@ export default class StakingStore extends Store {
this.numberOfStakePoolsFetched > 0
) {
this.cyclesWithoutIncreasingStakePools++;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

@daniloprates this is just FYI - no response required.
I believe this was a flaw in your original implementation.
We need to make sure the tracker is switched off only in case we don't see an increase in stake pools count for a six consecutive checks. Without this change we don't count consecutive ones at all - we count all of them.

gabriela-ponce
gabriela-ponce approved these changes Mar 5, 2021
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.

@daniloprates @nikolaglumac The previous issue is no longer reproducing, nice job 👍

@nikolaglumac nikolaglumac merged commit cccfb6e into develop Mar 5, 2021
@iohk-bors iohk-bors bot deleted the feature/ddw-554-show-loading-stake-pools-anytime-the-pools-are-loading branch March 5, 2021 13:42
@nikolaglumac nikolaglumac added release-4.0.2-FC3 Daedalus Flight and removed ⏳release-vNext labels Mar 15, 2021
@nikolaglumac nikolaglumac mentioned this pull request Mar 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikolaglumac nikolaglumac nikolaglumac left review comments

@gabriela-ponce gabriela-ponce gabriela-ponce approved these changes

@alexander-rukin alexander-rukin Awaiting requested review from alexander-rukin

@miorsufianiohk miorsufianiohk Awaiting requested review from miorsufianiohk

@DeeJayElly DeeJayElly Awaiting requested review from DeeJayElly

Assignees

@nikolaglumac nikolaglumac

@thedanheller thedanheller

Labels
feature release-4.0.2-FC3 Daedalus Flight
Projects
None yet
Milestone
No milestone
Development

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