@ltfschoen Did you find a way to manually reproduce the syncing behavior, so that we can test it?

@amaurymartiny I'll see if I can try to replicate the syncing behaviour with a test

@amaurymartiny Initially I thought I'd be best to actually write a unit test, but I couldn't get mocking to work https://github.com/paritytech/fether/compare/luke-320-sync-reset-send-form-fields-tests?expand=1.

So instead of writing a unit test I decided to just simulated it in the code. I changed the componentDidUpdate in RequireHealthOverlay.js as follows which triggers the overlay to switch on/off every 10 seconds. I found that even the proposed changes in this PR so far that it actually wasn't saving the current input field values. Then I found that each time the visibility changed it was re-mounting the TxForm.js component (which should have been obvious if I had looked closer at the render method of RequireHealthOverlay.js which doesn't render {this.props.children} when the this.state.visibility is false.

  componentDidUpdate () {
    const visible = this.state.visible;

    setTimeout(() => {
      this.setState({ visible: !visible });
    }, 10000);
  }

So I think what needs to be done is to start from scratch and simply update RequireHealthOverlay.js so we always render {this.props.children} but conditionally render the health overlay alert when visibility equals true (using CSS so the overlay floats over the children). I think such changes also relate to this issue #218

@ltfschoen Yes, let RequireHealthOverlay always render children is a good idea, I agree that this PR should be redone to make it that way. It will solve the original problem more cleanly.

Btw, initially, it was a real overlay (as in a layer on top of other components), but changed in #216 (cc @axelchalon was there a technical reason for that? I can't remember now). If you want to see the old CSS, look at https://github.com/paritytech/fether/pull/216/files#diff-e6912b69f6175ae14cf8eeb2923a231bL38

Edit: We could make RequireHealthOverlay a SUI Modal that takes all screen, to avoid custom CSS.

@ltfschoen Yes, let RequireHealthOverlay always render children is a good idea, I agree that this PR should be redone to make it that way. It will solve the original problem more cleanly.

Btw, initially, it was a real overlay (as in a layer on top of other components), but changed in #216 (cc @axelchalon was there a technical reason for that? I can't remember now). If you want to see the old CSS, look at https://github.com/paritytech/fether/pull/216/files#diff-e6912b69f6175ae14cf8eeb2923a231bL38

Edit: We could make RequireHealthOverlay a SUI Modal that takes all screen, to avoid custom CSS.

Yes, I think it should comprise:

• RequireHealthOverlay should always render this.props.children (so it doesn't re-mount child components causing input field values to reset) and when this.props.visibility is true then it should display a SUI modal. Note that I prepared a draft "custom CSS" example in the meantime here https://github.com/paritytech/fether/compare/luke-320-sync-reset-send-form-fields-alternative?expand=1

• Separate PR to incorporate use styled-components Add a UI library #158

I'm more in favour of non- full screen modal, I prefer just a small modal in the middle to get the message across to the user (or even better would be if it was out of the way all together, perhaps up in the top-right corner and using colour codes red/green like on the account list screen so you don't even have to read the connection error message, you just notice the colour change to 'red'), but allows the user can still continue modifying input fields values whilst they're waiting for the syncing or bad connection or whatever to restore (since with some transparency they can still select the input fields to edit them and mostly see what they're typing). Otherwise they lose time having to wait for the full screen modal to disappear.

Even if it's a bad connection, Parity's downloading, or there's no internet, or there's no peers, or it's syncing, ... i think the user shouldn't be interrupted from entering values into the input fields, nor from clicking the "Send" button

And after they click the "Send" button, we could use the full-screen modal for the "Submitted" modal (since doing so wouldn't interrupt the user from doing anything), and if there happens to be a connection issue we can show up in the top-right the connection issue and colour code

Even if it's a bad connection, Parity's downloading, or there's no internet, or there's no peers, or it's syncing, ... i think the user shouldn't be interrupted from entering values into the input fields, nor from clicking the "Send" button

@ltfschoen After some very long discussions between a lot of disagreeing people (in the Fether riot channel, surely in some issues and prs too), we decided that:

• We show a full modal on all screens that read/write blockchain state: tokens balances, send tx etc.
• We don't show a modal on other screens (account management), only a green/yellow/red button as you described.

Let's not revive that debate anymore :) (pm me if you want a summary of the reasons for this)

Btw, initially, it was a real overlay (as in a layer on top of other components), but changed in #216 (cc @axelchalon was there a technical reason for that? I can't remember now).

(edited)

Yes.

https://github.com/paritytech/fether/blob/3eb3ebf2f1d643bd0fed377505131adfa2099b8c/packages/fether-react/src/App/App.js#L63-L64

If we render components (such as AccountsList) that use light.js RPC decorators before we have setApi in light.js, then it errors out https://github.com/paritytech/js-libs/blob/master/packages/light.js/src/api.ts#L47

setApi is currently called only after Parity has been downloaded and launched https://github.com/paritytech/fether/blob/3eb3ebf2f1d643bd0fed377505131adfa2099b8c/packages/fether-react/src/stores/parityStore.js#L84

@axelchalon Ah, I think you're right.

@axelchalon @ltfschoen I still propose to go for an overlay as proposed by Luke. If the overlay makes the app break in App.js, we can for example add a prop mountChildren={true/false} in RequireHealthOverlay.

Replaced by this PR that builds a solution based on a SUI modal #355