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

Add network switcher to splash screen - Closes #1894 #1956

Merged

Conversation

Projects
None yet
3 participants
@michaeltomasik
Copy link
Contributor

commented Apr 25, 2019

What issue have I solved?

#1894

How have I implemented/fixed it?

Added network dropdown to splash screen

How has this been tested?

  1. Go to splash screen
  2. Change the network in the header
  3. Go to the login page and try to log in
  4. Check the network you are on.

Review checklist

michaeltomasik added some commits Apr 17, 2019

@michaeltomasik michaeltomasik requested a review from slaweet Apr 25, 2019

@michaeltomasik michaeltomasik self-assigned this Apr 25, 2019

@slaweet slaweet removed their request for review Apr 25, 2019

michaeltomasik added some commits Apr 25, 2019

@slaweet
Copy link
Member

left a comment

In every state, I found at least 3 design mismatches (colors, font size, margins, paddings, texts, background-color, ..). I will list everything specifically here after you fix the ones you can spot.

Screenshot 2019-04-26 at 10 11 30

Screenshot 2019-04-26 at 10 12 15

Screenshot 2019-04-26 at 10 13 26

Screenshot 2019-04-26 at 10 13 09

Show resolved Hide resolved src/components/headerV2/headerV2.js Outdated
Show resolved Hide resolved src/components/headerV2/headerV2.js Outdated
Show resolved Hide resolved src/components/headerV2/headerV2.js Outdated
this.setState({ showDropdown: value });
}

/* eslint-disable complexity */

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 26, 2019

Member

This eslint violation is telling you that most of the code you wrote should be a separate component <NetworkSwitcher> 😉 And in Header component it would be only {showNetwork && <NetworkSwitcher dark={dark}>}. NetworkSwitcher could have its own HOC and most of the methods that currently has this component.

But it's not super necessary to do this if it's not just an easy copy-paste.

slaweet and others added some commits Apr 26, 2019

Apply suggestions from code review
Co-Authored-By: michaeltomasik <shugo12345@gmail.com>
@slaweet
Copy link
Member

left a comment

Now it's a lot better, still, I see these miss-matches:

  • - "Connect" button not displayed if the input is empty
  • - font-size of "Connect" button
  • - padding-left of input
  • - padding-left of the error box
  • - color of the error icon
  • - background-color of the active item (background change is only hover effect)
  • - border-color of input
  • - padding-bottom of the hovered Custom node
  • - text of the button should change from "Connect" to "Connected" if connected

In terms of behavior:

  • - Clicking once opens the dropdown (works), clicking again should close it (nothing happens)
  • - Entering a wrong address, try to connect - get the error, select Mainnet, open dropdown again - now the input has the blue checkmark but contains invalid address
  • - The network selector should be also on the login page (is not)

michaeltomasik added some commits Apr 26, 2019

@michaeltomasik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

  • - optional show switcher after settings unlocked
  • - check mark when you type before clicking connect

michaeltomasik added some commits Apr 29, 2019

@@ -112,7 +112,7 @@ describe('Login Page', () => {
* Try to sign in to invalid custom node address
* @expect error popup is shown
*/
it('Log in to invalid address', () => {
xit('Log in to invalid address', () => {

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 30, 2019

Member

I don't see the reason to skip this test. It shouldn't be hard to adjust it to the new logic

selectedNetwork={peers.options.code || 0}
handleNetworkSelect={this.changeNetwork}
settingsUpdated={settingsUpdated}
showNetwork={this.showNetworkOptions()} />

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 30, 2019

Member

Why are all of these params duplicated in splashscreen and login page? The only param that I really see necessary to pass to HeaderV2 is dark={true}. Everything else should be inside HeaderV2.

michaeltomasik added some commits Apr 30, 2019

michaeltomasik added some commits Apr 30, 2019

@slaweet

slaweet approved these changes May 3, 2019

Copy link
Member

left a comment

Thanks, Mike 👍

@slaweet slaweet requested a review from Efefefef May 3, 2019

@Efefefef
Copy link
Contributor

left a comment

🐛 Used custom node address is saved only after login, should be saved after successful connect
Would be nice to have Enter working as Connect click in custom node address field

@yasharAyari Questions:

  1. The validation rules for custom node URL are not specified. And they were changed. Please specify.
  2. What should happen on app start if the last used network was custom node which is not available at the moment and connection failed?
@michaeltomasik

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@Efefefef I created the ticket for these things. Could you please approve it?
#1982

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@michaeltomasik Approve new ticket or this PR? I approve both

@Efefefef Efefefef added the ready label May 3, 2019

@michaeltomasik michaeltomasik merged commit 161de65 into development May 5, 2019

5 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.306%
Details
security/snyk - package.json (LiskHQ) No manifest changes detected

@michaeltomasik michaeltomasik deleted the 1894-add-network-switcher-to-splash-screen branch May 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.