feat - Fixes #293. Shows tx fee estimate and total tx amount. See issues in commit details #307

ltfschoen · 2018-12-18T20:17:33Z

Remaining Issues:
1. Update (28 Dec 2018): RESOLVED in commit 13fd9f6 - After entering a valid value and a recipient address, it works but the console displays error: Uncaught TypeError: Cannot read property 'resolve' of null at flush (index.js:62)
2. Update (28 Dec 2018): REQUIRES this PR to be merged fix: Fixes #320. Memoize form field values in state to prevent reseting on sync #326 - After I've enter a recipient and an amount and the Transaction Details have been calculated, then if the page refreshes (likely due to syncing) then the input values clear but the Transaction Details are still shown. Update: Note that I have raised this PR to prevent the input values from being cleared due to syncing fix: Fixes #320. Memoize form field values in state to prevent reseting on sync #326 so that this will no longer be an issue

Screenshot:

…es in commit details * After entering a valid value and a recipient address, it works but the console displays error: `Uncaught TypeError: Cannot read property 'resolve' of null at flush (index.js:62)` * On the 'Send THIBCoin' if you enter a valid value and a recipient address, the Tx Fee Estimate is shown as being the same value as the Amount, which is incorrect!

Tbaut · 2018-12-19T10:02:20Z

Thanks a lot @ltfschoen for the contributions. I'd like to keep the design as simple as possible, I'll try to come up with a Mockup soon to keep the screen simple because I find it overwhelming right now.

Tbaut · 2018-12-19T10:33:00Z

Here is the most simple way to show what you've built, but only to interested users.
We will need this "details" dropdown if we want to let users chose the gas price (cf #306 )

ltfschoen · 2018-12-19T20:34:32Z

@Tbaut I've pushed commits so it functions with show/hide details like in your wireframes as shown in screenshot below. I might darken the text to match though

I still have to resolve the two issues shown in the description.

Tbaut

that's cool, some nits

Tbaut · 2018-12-19T23:14:10Z

packages/fether-react/src/Send/TxForm/TxForm.js

+                          {estimatedTxFee &&
+                            !estimatedTxFee.isZero() && (
+                            <div>
+                              {showDetails && !isNaN(values.amount) ? (


Could you show the "details" all the time so that the "send" button doesn't jump?

Do you mean remove the 'Details' and 'Hide' buttons, and just show the details section all the time by default? Or retain the 'Details' and 'Hide' buttons functionality whilst preventing the "send" button from jumping when the user toggles one of the buttons?

I'll keep the 'Details' and 'Hide' button functionality, and just fix it so it doesn't jump each time you click 'Details' or 'Hide'

packages/fether-react/src/assets/sass/shared/_form.scss

Tbaut · 2018-12-19T23:20:08Z

packages/fether-react/src/Send/TxForm/TxForm.js

+    const renderCalculation = values => {
+      return `Calcs: (${estimatedTxFee
+        .div(new BigNumber(values.gasPrice.toString()))
+        .div(10 ** 9)} GWEI * ${new BigNumber(


This is in Wei

Tbaut · 2018-12-19T23:21:58Z

packages/fether-react/src/Send/TxForm/TxForm.js

+        .div(new BigNumber(values.gasPrice.toString()))
+        .div(10 ** 9)} GWEI * ${new BigNumber(
+        values.gasPrice.toString()
+      )} GWEI) / 10**18`;


I'd leave the / 10**18 because it makes things even more complex. Talking in ETH is fine.

…plex for users

…tton jump

ltfschoen · 2018-12-20T12:50:09Z

@Tbaut I've pushed more commits to address your latest review comments

I still have to resolve the three issues shown in the description.

…prevent wrapping in UI

… by using async await * Fixes error `Uncaught TypeError: Cannot read property 'resolve' of null at flush` that is generated after both valid amount and recipient values are entered

ltfschoen · 2018-12-27T21:57:15Z

@Tbaut I've pushed commits that fix the bugs so I think we can remove the "A3-inprogress" label now. Suggest merging PR #326 before this one so we can double check that the second issue gets resolved by that PR

amaury1093

I like the idea!

2 suggestions:

Can we make the details text selectable (nit)? It feels weird that it's not.
The text is not so clear to me right now, and there's a mistake: it's not 21,000 WEI, just 21,000 (units of gas).
I propose:

Estimate amount of gas: 21000
Fee: 0.000840000 ETH (gas estimate * gas price)
Total amount: 1.2000840000 ETH

amaury1093 · 2018-12-31T11:08:15Z

packages/fether-react/src/Send/TxForm/TxForm.js

  state = {
-    maxSelected: false
+    estimatedTxFee: ZERO,


Remove this from state. The estimated fee is final-form's values.gas time values.gasPrice, so no need for another state variable.

amaury1093 · 2018-12-31T11:08:45Z

packages/fether-react/src/Send/TxForm/TxForm.js

+  componentWillUnmount () {
+    // Avoids encountering error `Can't call setState (or forceUpdate)
+    // on an unmounted component` when navigate from 'Send Ether' to 'Send THIBCoin'
+    this.isCancelled = true;


Is we remove estimatedTxFee (see above), we don't need isCancelled anymore.

amaury1093 · 2018-12-31T11:09:44Z

packages/fether-react/src/Send/TxForm/TxForm.js

@@ -98,6 +125,56 @@ class Send extends Component {
      token
    } = this.props;

+    const { estimatedTxFee, showDetails } = this.state;
+
+    const renderFee = () => {


2 solutions here:

either create a variable called renderFee

or put this function as a class method.

Reason: now we're creating a new function on each render.

amaury1093 · 2018-12-31T11:09:59Z

packages/fether-react/src/Send/TxForm/TxForm.js

+        .toString()} ETH (Gas Limit * Gas Price)`;
+    };
+
+    const renderCalculation = values => {


same as above.

amaury1093 · 2018-12-31T11:10:04Z

packages/fether-react/src/Send/TxForm/TxForm.js

+      return `Calcs: (${gasLimitBn} WEI * ${gasPriceBn} GWEI)`;
+    };
+
+    const renderTotalAmount = values => {


same as above.

amaury1093 · 2018-12-31T11:10:10Z

packages/fether-react/src/Send/TxForm/TxForm.js

+        .toString()} ETH`;
+    };
+
+    const renderDetails = values => {


same as above.

amaury1093 · 2018-12-31T11:10:15Z

packages/fether-react/src/Send/TxForm/TxForm.js

+      )}`;
+    };
+
+    const showHideLabel = () => {


same as above.

amaury1093 · 2018-12-31T11:10:20Z

packages/fether-react/src/Send/TxForm/TxForm.js

+      );
+    };
+
+    const showDetailsLabel = () => {


same as above.

… prop to TxDetails

…textarea focus opacity

…iling 0's for say 0.0000000001

…total estimate

…l point in string

ltfschoen · 2019-01-04T09:24:42Z

@amaurymartiny I've addressed all your review comments

@Tbaut I've addressed all your review comments, except for:

The "send" button is still jumping when the "details" appears.

@Tbaut I just merged the latest changes from master and noticed that YJ has added an autoFocus on the Amount input field (which is now the second input field that is shown after re-ordering the input fields to match the wireframe you provided in this earlier comment #307 (comment)). I just wanted to confirm that this is correct?

ltfschoen · 2019-01-04T09:27:45Z

@Tbaut I've just had a look at the following review comment but I can't replicate it.

The "send" button is still jumping when the "details" appears.

Could you please check if it's still an issue for you with the latest changes?

Tbaut · 2019-01-04T09:59:53Z

Still happening on the latest iteration, the send button jumps when "details" appears.
There is also a slight horizontal jump of the whole windows (but this seems to be due to the automatic window size):

Tbaut

nit

packages/fether-react/src/Send/TxForm/TxForm.js

ltfschoen · 2019-01-04T12:40:52Z

@Tbaut I've pushed commits addressing your issues. I noticed it on mine as well but hadn't looked close enough, and on closer inspection I found that it was the scroll bar appearing very briefly that caused the jumping effect. I found that I just needed to hide the scroll on the body element, which I've included in one of the commits

…licker on/off when TxDetails toggled

amaury1093

lgtm! imo there's a script that is actually not needed, but I'll wait for @ltfschoen's answer.

amaury1093 · 2019-01-04T16:50:48Z

packages/fether-react/src/Send/TxForm/TxDetails/TxDetails.js

+        ),
+        'ether'
+      )
+        .toFixed(18)


If you remove this line, then .toString() actually already removes trailing zeroes, and I believe removeTrailingZeros is not needed then?

Yes removing that script and removing .toFixed(18) fixed it

…ng zeros

…oved into footer" * Move "Details" / "Hide" buttons into the form-nav footer section * TxDetails now appears above the footer area above other input fields without causing the screen height to increase * Background of the TxDetails darkened so it stands out from surroudings * Change "Details" arrow to point upwards since TxDetails now shown above * Change "Hide" arrow to point downward since TxDetails now shown above * Rename classnames to `_` or `-` more consistently * Note: To quickly display the 'Details' button so you can click it change TxForm.js as follows: ``` {/* {valid && this.estimatedTxFee(values) ? ( */} <TxDetails // estimatedTxFee={this.estimatedTxFee(values)} estimatedTxFee={new BigNumber('8100000')} token={token} // values={values} values={ { amount: new BigNumber('0.1'), gasPrice: new BigNumber('21000') } } /> {/* ) : null} */}

ltfschoen · 2019-01-04T22:15:29Z

Based on discussion with @Tbaut I've created a separate branch and this commit bd56db6 that includes:

Displays the "Details / Hide" buttons in the footer so when they appear they do not cause the height of the window to change (same horizontal alignment as the 'Send' / 'Checking...' button)
When "Details" button is clicked it displays the transaction details on top of the other fields so it doesn't cause the height of the window to change.

If we'd like this option then it's probably worth also refactoring the code to try and use a SUI popup/modal. What do you think?

Screenshot:

With the code in that branch/commit the only issue that appears to still need fixing is when it changes to 'Checking...' instead of 'Send' it causes both the "Details / Hide" buttons and the transaction details popup to shift to the left (since the text of 'Checking...' has more characters and makes the button wider requiring more space).

Screenshot with bug:

Tbaut · 2019-01-06T14:50:43Z

I think there was a misunderstanding. My request was actually very small and most probably just needed a CSS trick. What you did before with a slide down was totally fine and matched the mockup we agreed on.

The only problem I saw was with the jump of the "Send" button when the details button appears.
An image is worth 1000 words.

We currently have a transition from 1 -> 2, the appearance of "Details" toggle button makes the windows longer, that's what I meant with the jump of the "Send" button.
I wish to make sure the space taken by the "Details" button is already reserved (green area hilighted in 3) so that we would have a transition from 3 -> 2 (the green area in 3 should obviously be white).

Please PM if that's not clear.
We could also simply show a disabled "Details" button when not available.

For the sake of completeness, here is my reasoning:

it's fine for things to change a lot (such as a window resize) when the user expects it, e.g user click on a "Details" button -> the windows shows a new section and resizes
it's not fine if the user doesn't expect it, e.g the user enters an amount and an unexpected "Details" button appears which makes the window resize.

…details buttons to footer * Move toggle details buttons and associated methods and showDetails state into nav of TxForm component * Pass showDetails down as props to TxDetails * Remove previous attempt to display TxDetails like a popup above other fields (i.e. remove form-details-wrapper and -details-value styles)

ltfschoen · 2019-01-06T18:44:53Z

@Tbaut I've push a commit that's a slight alternative to what you've shown. It gets rid of the "green" area (shown in your image) all together so we don't have to unnecessarily reserve that entire space just for the little "Details / Hide" toggle button. It involves moving the "Details / Hide" toggle button div into the footer nav bar so it appears inline with the "Send / Checking..." button (so we're grouping our buttons together in the footer navbar). Whilst the form input fields aren't valid or it's still validating or if its still calculating the gas then it doesn't show the "Details / Hide" toggle buttons or the details section at all.

Screenshots:

ltfschoen · 2019-01-06T19:22:29Z

@Tbaut If your not happy with the above alternative just let me know. It'd be a quick change to do as you've requested by reserving an area above the "Send / Checking..." button and move the "Details / Hide" toggle button there (but I was hoping that squeezing all the buttons together into the footer nav would be more appealing).

Currently it does not showing the "Details / Hide" toggle buttons or the details section at all when it's not "ready" (i.e. the form input fields aren't valid or it's still validating or if its still calculating the gas), and I'd suggest retaining this functionality

Tbaut

Thanks a lot, that's fine like this.
We can improve the behaviour when the details section is shown and you change the amount, as the whole section gets hidden and shown again, but that can be addressed in another PR :)

…etails. * Add styled-components dependency to start introducing it to components instead of Sass * Wrap App.js with styled-components ThemeProvider and inject `fetherTheme` prop colours so they are available to child components. * Added styled-components' GlobalStyle https://www.styled-components.com/docs/faqs boilerplate as child of ThemeProvider so global styles may be defined here * Replace assets/sass/layouts/_wrapper.scss with styled-components including: * DivContent is declared in App.js since it is only used here * DivWindow is declared in App.js since it is only used here * DivWindowContent is declared in shared folder since used by different components * Removed assets/sass/layouts/_wrapper.scss. Noting that `.connector` class is not used * Use styled-components in TxForm and TxDetails components (that were introduced in PR #307). * Add a style.js file in the same directory of the component where styled-components are used * Add styled-components theme animations folder with a SlideInLeft animation and introduce a theme to TxDetails * Add keyframes animation to TxDetails component (i.e. slide-in animation that's used in the details section that appears when you click Details/Hide button on the Send Ether/THIBCoin page) * Fix `faint` style implementation to use `rgba` with styled-components * Note that I decided not to use defaultProps feature of styled-components as it bloats the code too much * Note that currently we are using Sass _variables.scss for colour variables, and since many components still use some of the Sass variables we cannot remove them yet. * Note that the original branch where this work was done is https://github.com/paritytech/fether/compare/luke-293-show-tx-fee-styled-components?expand=1, but since multiple PRs have since been merged it was easier to just code again using it as reference * Disadvantage of styled-components appears to be when you go to debug components in Dev Tools > Elements. The classNames appear as a hash `<div class="sc-EHOje dGUdfn">` instead of the classname!!

ltfschoen added 6 commits December 19, 2018 20:14

Merge branch 'master' into luke-293-show-tx-fee

f4d6784

review-fix: Swap order that Amount and Gas Price are shown

b0ce528

review-fix: Rename Gas Price back to Transaction Speed

9f3b3c6

review-fix: Combine Tx Fees and Total Amount into a Tx Details component

ba122f2

review-fix: Squeeze calculation into details section and refactor code

c491cba

refactor: Rename Calculation to Calcs so fits in better

0be070d

Tbaut reviewed Dec 19, 2018

View reviewed changes

ltfschoen mentioned this pull request Dec 20, 2018

Add a scientific notation toggle button. Show non-scientific notation equivalent. #316

Closed

ltfschoen added 4 commits December 20, 2018 13:31

review-fix: Gas Price of 21000 is measured in Wei not Gwei

48136d1

review-fix: Remove the division by 10**18 since makes it look too com…

eebf01d

…plex for users

review-fix: Darken the colour of the Details section placeholder text

aa689f4

review-fix: Toggling the Details/Hide buttons no longer makes Send bu…

6964453

…tton jump

merge latest from master and fix merge conflict

18dab6e

Tbaut added the A3-inprogress label Dec 20, 2018

ltfschoen added 4 commits December 26, 2018 07:45

merge latest from master

d053f7d

refactor: Make it gas limit clearer in calcs. Round to 9 decimals to …

d9e6a0a

…prevent wrapping in UI

refactor: Simplify syntax of call to function

0e86a7a

fix: Fix unresolved promise bug when enter valid amount and recipient…

13fd9f6

… by using async await * Fixes error `Uncaught TypeError: Cannot read property 'resolve' of null at flush` that is generated after both valid amount and recipient values are entered

ltfschoen changed the title ~~WIP - Fixes #293. Shows tx fee estimate and total tx amount. See issues in commit details~~ feat - Fixes #293. Shows tx fee estimate and total tx amount. See issues in commit details Dec 27, 2018

amaury1093 added A0-pleasereview and removed A3-inprogress labels Dec 31, 2018

amaury1093 reviewed Dec 31, 2018

View reviewed changes

amaury1093 added A5-grumble and removed A0-pleasereview labels Dec 31, 2018

ltfschoen added 8 commits January 4, 2019 17:29

fix: Fix typo

44fa269

review-fix: Pass down estimatedTxFee value instead of the function as…

7a53f64

… prop to TxDetails

review-fix: Remove useless prop that is no longer used

e7af4eb

review-fix: Change TxDetails text to be full opacity to match normal …

c331736

…textarea focus opacity

review-fix: Show values that have Wei decimal places. Need remove tra…

7af4838

…iling 0's for say 0.0000000001

review-fix: Remove trailing zeros from big number decimal values for …

7b79b80

…total estimate

fix: Do not process removing trailing zeros when more than one decima…

b73ecab

…l point in string

merge latest from master

063215b

Tbaut reviewed Jan 4, 2019

View reviewed changes

packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved

refactor: Move autofocus from the Amount to the To input field on TxForm

e5e3a76

review-fix: Hide vertical overflow on body element so scrollbar not f…

2a9403b

…licker on/off when TxDetails toggled

amaury1093 approved these changes Jan 4, 2019

View reviewed changes

amaury1093 added A8-looksgood and removed A5-grumble labels Jan 4, 2019

ltfschoen added 2 commits January 5, 2019 07:08

review-fix: Remove toFixed(18) so do not need script to remove traili…

a684de2

…ng zeros

Tbaut approved these changes Jan 6, 2019

View reviewed changes

Tbaut merged commit e7437e0 into master Jan 6, 2019

ltfschoen mentioned this pull request Jan 13, 2019

feat: Relates to #158. Add styled-components UI library. See commit details. #357

Closed

Tbaut deleted the luke-293-show-tx-fee branch January 18, 2019 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat - Fixes #293. Shows tx fee estimate and total tx amount. See issues in commit details #307

feat - Fixes #293. Shows tx fee estimate and total tx amount. See issues in commit details #307

ltfschoen commented Dec 18, 2018 •

edited

Tbaut commented Dec 19, 2018

Tbaut commented Dec 19, 2018

ltfschoen commented Dec 19, 2018 •

edited

Tbaut left a comment

Tbaut Dec 19, 2018

ltfschoen Dec 20, 2018

ltfschoen Dec 20, 2018 •

edited

Tbaut Dec 19, 2018

Tbaut Dec 19, 2018

ltfschoen commented Dec 20, 2018

ltfschoen commented Dec 27, 2018 •

edited

amaury1093 left a comment

amaury1093 Dec 31, 2018

amaury1093 Dec 31, 2018

amaury1093 Dec 31, 2018

amaury1093 Dec 31, 2018

amaury1093 Dec 31, 2018

amaury1093 Dec 31, 2018

amaury1093 Dec 31, 2018

amaury1093 Dec 31, 2018

ltfschoen commented Jan 4, 2019

ltfschoen commented Jan 4, 2019

Tbaut commented Jan 4, 2019 •

edited

Tbaut left a comment

ltfschoen commented Jan 4, 2019

amaury1093 left a comment

amaury1093 Jan 4, 2019 •

edited

ltfschoen Jan 4, 2019

ltfschoen commented Jan 4, 2019 •

edited

Tbaut commented Jan 6, 2019

ltfschoen commented Jan 6, 2019 •

edited

ltfschoen commented Jan 6, 2019 •

edited

Tbaut left a comment

feat - Fixes #293. Shows tx fee estimate and total tx amount. See issues in commit details #307

feat - Fixes #293. Shows tx fee estimate and total tx amount. See issues in commit details #307

Conversation

ltfschoen commented Dec 18, 2018 • edited

Tbaut commented Dec 19, 2018

Tbaut commented Dec 19, 2018

ltfschoen commented Dec 19, 2018 • edited

Tbaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltfschoen Dec 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltfschoen commented Dec 20, 2018

ltfschoen commented Dec 27, 2018 • edited

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltfschoen commented Jan 4, 2019

ltfschoen commented Jan 4, 2019

Tbaut commented Jan 4, 2019 • edited

Tbaut left a comment

Choose a reason for hiding this comment

ltfschoen commented Jan 4, 2019

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Jan 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltfschoen commented Jan 4, 2019 • edited

Tbaut commented Jan 6, 2019

ltfschoen commented Jan 6, 2019 • edited

ltfschoen commented Jan 6, 2019 • edited

Tbaut left a comment

Choose a reason for hiding this comment

ltfschoen commented Dec 18, 2018 •

edited

ltfschoen commented Dec 19, 2018 •

edited

ltfschoen Dec 20, 2018 •

edited

ltfschoen commented Dec 27, 2018 •

edited

Tbaut commented Jan 4, 2019 •

edited

amaury1093 Jan 4, 2019 •

edited

ltfschoen commented Jan 4, 2019 •

edited

ltfschoen commented Jan 6, 2019 •

edited

ltfschoen commented Jan 6, 2019 •

edited