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.

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

Replace old delegates page with the new one - Closes #1975 #2080

Merged
merged 32 commits into from May 31, 2019

Conversation

Projects
None yet
3 participants
@slaweet
Copy link
Member

commented May 29, 2019

What issue have I solved?

#1975

How have I implemented/fixed it?

  • I changed the new delegates page to be the one in the top bar, always displayed.
  • Removed the components for the old delegates page
  • Fixed some bugs in the new delegates section from previous PRs
  • I also changed CustomRouter component not to render anything if network or account is loading, because the delegate list was not fetched if delegates page was visited directly with autologin. This will prevent a lot of issues that we had on pages that tried to fetch data before the network and account was set up. Not having this in the first place was IMO the reason why we have conditions like this
    if (!liskAPIClient) return;

How has this been tested?

Review checklist

@slaweet slaweet self-assigned this May 29, 2019

slaweet added some commits May 29, 2019

🐛 Prevent rendering of pages when network or account not ready
it solves loading delegates on delegate page

@slaweet slaweet force-pushed the 1975-replace-old-delegates-with-new branch from fcd2e5d to d046176 May 29, 2019

@slaweet slaweet changed the base branch from 1973-implement-voting-submitted-page to development May 29, 2019

slaweet added some commits May 29, 2019

@slaweet slaweet marked this pull request as ready for review May 29, 2019

slaweet added some commits May 29, 2019

@slaweet slaweet force-pushed the 1975-replace-old-delegates-with-new branch from d888192 to 0bea737 May 29, 2019

Put back nextBtn selector
... as it was used in other e2e tests
@Efefefef

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

🐛 Onboarding: Info text should be narrower like in designs
🐛 Onboarding: Previous and Next buttons are swapped in design. Looks weird though. @yasharAyari ?
🐛 Onboarding: Mistakes. Why texts are not taken from the file with corrected wordings?
Want to dig deeper? We got you covered. You can read more about Lisk’s delgates, voting mechanism and benefits in a dedicated section of Lisk’s help centre.
🐛 Table: Load more should not be available on Votes/Not voted tabs if there is less than 100 delegates to show
🐛 Table: Wording: No search result in given criteria. : should be results
🐛 Table: No spacing between number and LSK
🐛 Table: Click on a row in Vote mode doesn't work as check/uncheck. Why to have hover state then?
🐛 Table: Font of Rank looks too small comparing to checkbox size, other columns as well - not like in design
🐛 Voting submitted: Voting submitted You will be notified when your votes are forged.: Why Voting not Votes. Notified how? Icon diff design
🐛 Tooltips: why do we have only for 'Total actions' and 'Transaction fee'?

  1. Why we have delegateOnboarding as localStorage key, not like settings object key like for the other flags?
  2. Should we support launch protocol voting as well?

slaweet added some commits May 31, 2019

@slaweet

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

🐛 Onboarding: Info text should be narrower like in designs

Fixed

🐛 Onboarding: Previous and Next buttons are swapped in design. Looks weird though. @yasharAyari ?

The order was weird, in latest design in inVission it's as implemented https://projects.invisionapp.com/d/main#/console/17570736/365903447/inspect

🐛 Onboarding: Mistakes. Why texts are not taken from the file with corrected wordings?
Want to dig deeper? We got you covered. You can read more about Lisk’s delgates, voting mechanism and benefits in a dedicated section of Lisk’s help centre.

Good catch. Fixed from https://docs.google.com/spreadsheets/d/1157pqdDDu6AVVJNHOv4VKlf6tRSiy0Wm753Ovbcp5as/edit#gid=0

🐛 Table: Load more should not be available on Votes/Not voted tabs if there is less than 100 delegates to show

Fixed.

🐛 Table: Wording: No search result in given criteria. : should be results

Fixed

🐛 Table: No spacing between number and LSK

Fixed

🐛 Table: Click on a row in Vote mode doesn't work as check/uncheck. Why to have hover state then?

Fixed. Whole row click works now.

🐛 Table: Font of Rank looks too small comparing to checkbox size, other columns as well - not like in design

Fixed

🐛 Voting submitted: Voting submitted You will be notified when your votes are forged.: Why Voting not Votes. Notified how? Icon diff design

Re: Icon diff: Dominic changed the success/error icons: https://projects.invisionapp.com/d/main#/console/17570736/365994789/preview

'Voting submitted' is approved copy, IMO fine, so I kept it.

🐛 Tooltips: why do we have only for 'Total actions' and 'Transaction fee'?

Fixed. Added tooltip for "Productivity" and "Vote weight"

  1. Why we have delegateOnboarding as localStorage key, not like settings object key like for the other flags?

I guess it's because then the onboarding component needs HOC to connect to store and toolbox components shouldn't need HOC.

  1. Should we support launch protocol voting as well?

Launch protocol should work.

slaweet added some commits May 31, 2019

🐛 Fix loading glitch
There was a short time between network set and account loading starting
so I use peers store instead of network store

@slaweet slaweet force-pushed the 1975-replace-old-delegates-with-new branch from 2008fb0 to f01e7d9 May 31, 2019

@slaweet slaweet requested review from massao and yasharAyari May 31, 2019

@yasharAyari
Copy link
Member

left a comment

good job @slaweet. 👍

@slaweet slaweet merged commit 0a5ec31 into development May 31, 2019

4 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
coverage/coveralls Coverage remained the same at 94.846%
Details
@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

🏅

@slaweet slaweet added the ready label Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.