Allow input amount in fiat if selected - payment + invoice #465

valentinewallace · 2018-07-19T08:35:42Z

Closes #461

The width of the input field is kinda weird still. I think we should switch to a monospace font for the amounts, which would help a lot for amounts like "11111" etc.

tanx

We'll also need a new helper function that calculates the satoshis from fiat. Right now we assume payment.amount is in a btc unit:
https://github.com/lightninglabs/lightning-app/pull/465/files#diff-c103796afd01e866f6af483b8930cf32R88

tanx · 2018-07-19T09:09:56Z

src/action/payment.js

  setAmount({ amount }) {
+    if (amount < 0) {
+      amount = 0;
+    }


Amount is set by the RN TextInput and is a string in the format 0.01.

tanx · 2018-07-19T09:11:30Z

src/component/field.js

-    style={[amountStyles.input, style]}
-    charWidth={46}
+    style={[amountStyles.input, { textAlign: fiat ? 'left' : 'right' }, style]}
+    charWidth={40}


Heads up that the 46 pixels is required for the TextInput width to scale correctly.

tanx · 2018-07-19T09:12:19Z

src/component/field.js

  <HorizontalExpandingTextInput
-    style={[amountStyles.input, style]}
-    charWidth={46}
+    style={[amountStyles.input, { textAlign: fiat ? 'left' : 'right' }, style]}


I wouldn't change the style in the first iteration. Important part is getting the logic right. We can revisit styling later.

valentinewallace · 2018-07-20T02:58:57Z

@tanx should I also be adding this to the Create Channel page?

tanx · 2018-07-23T09:18:48Z

src/view/invoice.js

  },
+  fiatUnit: {
+    color: color.blackDark,
+  },


Also not needed for now.

For me, when I take this styling out it looks like this:

Sorry. That was an outdated comment.

tanx · 2018-07-23T09:20:44Z

src/view/invoice.js

+  const satoshis = fiatToSatoshis(fiatAmount, settings);
+  const amount = toAmount(satoshis, settings.unit);
+  invoice.setAmount({ amount });
+};


Since when do we put logic in react components? 🙃This logic is unit testable and needs to happen in the actions.

tanx · 2018-07-23T09:21:31Z

src/view/invoice.js

+          </BalanceLabelNumeral>
          <AmountInputField
            autoFocus={true}
-            value={store.invoice.amount}


All our TextInputs are currently controlled react components. We need to add this again.

tanx · 2018-07-23T09:24:41Z

src/view/invoice.js

            autoFocus={true}
-            value={store.invoice.amount}
-            onChangeText={amount => invoice.setAmount({ amount })}
+            onChangeText={amount => setAmount(amount, invoice, store.settings)}


Please revert back store.invoice.amount should reflect what actually rendered on screen. The actual satoshi value that is passed to the grpc api should be handled in a different variable.

tanx · 2018-07-23T09:24:54Z

src/view/invoice.js

-          <BalanceLabelUnit style={styles.unit}>{store.unit}</BalanceLabelUnit>
+          <BalanceLabelUnit style={styles.unit}>
+            {store.unitLabel}
+          </BalanceLabelUnit>


tanx · 2018-07-23T09:35:41Z

src/view/invoice.js

+            ]}
+          >
+            $
+          </BalanceLabelNumeral>


What happens for Euro or GBP? Probably best to rename the current fiat display names to displayLong in config.js like for the btc units and add a short display for the currency symbols being displayed.

tanx · 2018-07-23T09:36:04Z

src/view/pay-bitcoin.js

+              ]}
+            >
+              $
+            </BalanceLabelNumeral>


Same as above...

tanx · 2018-07-23T09:36:14Z

src/view/pay-bitcoin.js

-              onChangeText={amount => payment.setAmount({ amount })}
+              onChangeText={amount =>
+                setAmount(amount, payment, store.settings)
+              }


Same as above...

tanx · 2018-07-23T09:36:22Z

src/view/pay-bitcoin.js

+  const amount = toAmount(satoshis, settings.unit);
+  payment.setAmount({ amount });
+};
+


Same as above...

tanx · 2018-07-23T09:37:43Z

test/unit/helper.spec.js

+      expect(num, 'to equal', 145030);
+    });
+  });
+


tanx

@valentinewallace I did a pretty big refactoring, but the code is much cleaner now.

The toAmount and toSatoshis can handle both fiat/btc values. This way I could replace all fiat specific helpers. Also, the changes to the actions/computed modules is much more minimally invasive like this.
The create-channel view also supports fiat input now.

Take a look. Good the merge from my side.

valentinewallace requested a review from tanx July 19, 2018 08:35

tanx suggested changes Jul 19, 2018

View reviewed changes

valentinewallace force-pushed the fiat-amount-input branch from 78ab78f to 7a192cd Compare July 20, 2018 02:55

tanx suggested changes Jul 23, 2018

View reviewed changes

valentinewallace added 3 commits July 24, 2018 19:04

Allow input amount in fiat if selected - payment + invoice

9b2095e

Fix logic and add tests for display fiat on amt input

a19e3be

Fix logic for setting payment.amount + displaying correct fiat label

a827c2f

valentinewallace force-pushed the fiat-amount-input branch from 7a192cd to a827c2f Compare July 25, 2018 08:11

tanx added 9 commits July 25, 2018 16:35

Add unitFiatLabel

3fb9d2b

Removed unused fiatUnit style

39a6647

Add unit tests for unitFiatLabel

ab1d0d7

Simplify toSatoshis to handle both btc/fiat amounts

1940a3d

Refactor toAmount to support btc/fiat values

5e4d292

Fix use of internal helper function calls

feda5f1

Cleanup helper usage in action/computed modules

ae791c5

Use fiat in create channel view

144c0b2

Cleanup invoice action helper usage

f926eae

tanx approved these changes Jul 26, 2018

View reviewed changes

Fix comment indenting

a65e3e3

valentinewallace merged commit 04a4e7f into master Jul 26, 2018

valentinewallace deleted the fiat-amount-input branch July 26, 2018 23:14

Allow input amount in fiat if selected - payment + invoice #465

Allow input amount in fiat if selected - payment + invoice #465

Uh oh!

Conversation

valentinewallace commented Jul 19, 2018

Uh oh!

tanx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanx Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tanx Jul 23, 2018 •

edited

Loading

tanx left a comment •

edited

Loading