Bug fix NaN displaying in GUI

[secondl1ght]

Bug fix for issue #382, this should solve the problem of NaN displaying in the GUI and blocking users from inputting an allowance. It will also prevent NaN from being set as the budget state.

Please ignore the unrelated commit that I added and removed, I accidentally made a different PR from the master branch of my Alby fork and I have a bunch of PRs waiting right now so I can't delete my fork and re-fork yet to have a clean master branch (if that makes sense). The commit that matters for this PR is the Bug fix one.

Thanks :D

[dylancom]

Thanks for the PR and finding this issue!
As we currently use placeholders for the absence of a value, I think we need to make sure that the input field can also be empty (instead of becoming 0).

PS. This is also broken btw. in screens/ConfirmPayment

My suggestions:

• Change the input type to number in <CurrencyInput />
• Then you can get rid of parseInt and use: event.target.valueAsNumber directly.
• Then the onChange could be something like this:
  setBudget(!isNaN(event.target.valueAsNumber) ? event.target.valueAsNumber : undefined);
  so the field can also be empty.

Bonus:
-> Like we do in other screens we can disable the Save button until the budget contains a value.

[secondl1ght]

Awesome thanks for the suggestions! This makes more sense for sure, I have made the changes you suggested and also disabled the button when the input is empty. Everything seems to be working now as you described and this also fixed the ConfirmPayment screen input as well.

I tried adding the disable button feature to the ConfirmPayment screen but was not able to get it working yet, I think it is a bit more complicated because that is a Class component instead of a Function one. One thing I tried was: disabled={this.state.loading || this.state.budget === NaN} but that didn't work.

[dylancom]

Great work and thanks again for finding this issue! I think disabling the confirm in ConfirmPayment is not necessary.

I have 1 suggestion left though as inside ConfirmPayment budget can still be set to NaN. We can for e.g. change line 49: this.setState({ budget: parseInt(satoshi) }); to this.setState({ budget: parseInt(satoshi) || undefined });

So budget won't be set to NaN in case the field is left empty. What you think?

[secondl1ght]

Thanks and no problem I am glad I was able to find a bug. :)

I can make that change for sure, but for some reason changing the input type to number in <CurrencyInput /> seems to have solved the budget being set to NaN, when the input field is blank it displays the placeholder. I am not sure why but maybe there is logic built in for blank inputs when you set type to number. Also I was just curious so I went back and reverted my changes to <AllowanceMenu /> as well so that the only change I made to the code is type="number" and that seems to also have fixed the NaN issue in the Allowance Menu... I haven't console.logged to see what the budget is actually being set to when the input is empty though.

I just thought I should let you know about this, but do you still want me to go ahead with the other changes we made? That seems to be the more explicit way of handling this instead of just relying on type="number". What do you think?

[dylancom]

Yeah so it's currently working fine as NaN won't show up with type="number" and when submitting there is a check if budget is "truthy". But I think we don't want the state to be set to NaN (in the background) anyway so to be more correct I'd say we add "budget: parseInt(satoshi) || undefined".

Might prevent future errors.

[secondl1ght]

Sounds good thanks a lot for working through this with me, it is a great learning opportunity for me! I made the change and am running into a Type error now:

src/app/components/AllowanceMenu/index.tsx(107,19): error TS2345: Argument of type 'number | undefined' is not assignable to parameter of type 'SetStateAction<number>'.
  Type 'undefined' is not assignable to type 'SetStateAction<number>'.
husky - pre-commit hook exited with code 2 (error)

Still new to TypeScript but I tried changing the Props type in the AllowanceMenu to totalBudget: number | undefined but that did not seem to fix it.

[dylancom]

No problem: const [budget, setBudget] = useState<number | undefined>(0);
Wasn't aware this problem is also in AllowanceMenu. (Was referring to ConfirmPayment).
Can you disabled "Save" there too if no value is defined? (this will prevent bugs)

[secondl1ght]

Well its weird because src/app/screens/ConfirmPayment/index.jsx is not even a TypeScript file, and I did not get a Type error when committing the changes to AllowanceMenu, budget was able to be undefined there without errors. And I dont know why the error message is pointing me to the AllowanceMenu file when the change I am making is in the ConfirmPayment file... sorry if this is confusing hopefully I am explaining my steps clearly.

Update: Those changes did work though, you can take a look at them in the latest commit.

[dylancom]

Thanks!