Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

RequireHealthOverlay: App (require connection) and TxForm & Signer (require sync) #216

Merged
merged 7 commits into from
Nov 12, 2018

Conversation

axelchalon
Copy link
Contributor

Part of #159

@axelchalon axelchalon requested a review from amaury1093 October 27, 2018 10:21
typo, if there is no peer, it's not plural.
@Tbaut
Copy link
Collaborator

Tbaut commented Oct 27, 2018

It's working great, one "weird" thing is that when the node not only disconnects, but it is turned off (by me), Fether turns totally white:
If that can help, the console shows ws.js:102 WebSocket connection to 'ws://127.0.0.1:8546/' failed: Error in connection establishment: net::ERR_CONNECTION_REFUSED because I turned off the light node.

(filed in #218)

@amaury1093
Copy link
Collaborator

(didn't try yet, but)

Let's say for some reason the light client takes 5 minutes to sync. If we do target aunts and grandmas, they will click around. 1/ they will see wrong balance at some point (might not be a big deal) and 2/ they might send txs before the light client is sycned. 2 should definitely be disabled.

@amaury1093
Copy link
Collaborator

amaury1093 commented Nov 6, 2018

Linked to #228. When we are not sync, do we allow:

  • users to see their list of accounts? Yes, this probably won't slow down LC sync too much.
  • users to see their balances? No, as this slows down the LC sync. We can show a loading placeholder/the oldest value we got.
  • users to send tx? No.

So we basically only allow users to see their accounts, when we're syncing. Imo we can keep the overlay in this case.

EDIT: Rethought about it: it's still doable without the overlay. We just need to define what we disable when we're not sync (send tx button, input forms?) cc @Tbaut

@Tbaut
Copy link
Collaborator

Tbaut commented Nov 6, 2018

The problem is somewhat more complex than:

  • we sync
  • we are synced

for me it's more:

  • we are synced
  • we sync and are far from the top (eg client just started and it might take a couple sec/min)
  • we sync but we're not far from the top at all (eg client was running low on peer, temporary bad internet connection..)

^ the 3rd case is where the overlay was particularly annoying.

users to see their balances? No, as this slows down the LC sync. We can show a loading placeholder/the oldest value we got.

Can we have some kind of cache/older state eventually?
+1 for "the oldest value we got".
For a better UX we could add a text like "Syncing, account information might be outdated" at the bottom or on a tooltip when hovering the red/orange button.

@axelchalon axelchalon requested review from Tbaut and amaury1093 and removed request for amaury1093 November 8, 2018 17:42
@axelchalon axelchalon changed the title Remove health Overlay Keep Overlay on TxForm & Signer only (require sync) Nov 8, 2018
@Tbaut
Copy link
Collaborator

Tbaut commented Nov 8, 2018

Nice! it even shows the past balance of ETH.
Can we also have the same behaviour for the tokens?

When I launch Fether, I see a blank window for ~10s before it shows the accounts etc.
Should we have an overlay there with a message saying that we're launching Fether :) ? More than a sec without message feels long, I originally thought Fether had crashed!

image

@axelchalon
Copy link
Contributor Author

axelchalon commented Nov 9, 2018

Just realized that the fields (user input) will be reset if the error shows up, not sure how annoying this is

Nice! it even shows the past balance of ETH.
Can we also have the same behaviour for the tokens?

Wasn't intentional and not sure how to reproduce this non-intentionality for tokens :P

When I launch Fether, I see a blank window for ~10s before it shows the accounts etc.
Should we have an overlay there with a message saying that we're launching Fether :) ? More than a sec without message feels long, I originally thought Fether had crashed!

Good catch, will fix now

@axelchalon
Copy link
Contributor Author

ready for testing

This PR:

  • If not connected to the node, show a full-screen overlay
  • If just not synced, show a partial overlay on the TxForm/Signer screens

@axelchalon axelchalon changed the title Keep Overlay on TxForm & Signer only (require sync) RequireHealthOverlay: App (require connection) and TxForm & Signer (require sync) Nov 9, 2018
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Working well :) I'll create a dedicated issue for the balance

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants