Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE #3213] Allow to reset gas details #3272

Merged
merged 1 commit into from
Feb 13, 2018
Merged

[ISSUE #3213] Allow to reset gas details #3272

merged 1 commit into from
Feb 13, 2018

Conversation

jeluard
Copy link
Contributor

@jeluard jeluard commented Feb 9, 2018

fixes #3213

Summary:

Allow to reset gas details to default value

Steps to test:

  • Open Status
  • Change wallet fee in send screen
  • Validate reset button works as expected

status: ready

@jeluard jeluard self-assigned this Feb 9, 2018
@jeluard jeluard added the wallet label Feb 9, 2018
:gas-price ethereum/default-gas-price})

(defn transaction-send-default []
(merge (gas-default)
{:symbol :ETH}))
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it a function? seems like it can be easily transformed to variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this change to prepare for the move to a computed gas price (#3241)

Copy link
Member

Choose a reason for hiding this comment

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

could you leave a comment?

@@ -42,7 +45,7 @@
:tags []
:sync-state :done
:wallet.transactions constants/default-wallet-transactions
:wallet {:send-transaction transaction-send-default}
:wallet {:send-transaction (transaction-send-default)}
Copy link
Member

Choose a reason for hiding this comment

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

why it became a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this change to prepare for the move to a computed gas price (#3241)

@jeluard
Copy link
Contributor Author

jeluard commented Feb 9, 2018

@alwx @flexsurfer Reverted to def

@annadanchenko annadanchenko self-assigned this Feb 9, 2018
@annadanchenko
Copy link

annadanchenko commented Feb 9, 2018

Issue 1: Recipient field content is cleared on Send Transaction screen after using Reset to Default on Transaction Fee screen. I would expect that it resets only values related to this screen and keeps other value untouched.

Found on iOS TF (from 00:19) https://app.testfairy.com/projects/4803590-status/builds/7671104/sessions/4/?accessToken=/dVbNaVMuWDVKuGWNm9HzN/it4g

Steps:

  • Open Wallet
  • Tap on Send transaction
  • Choose Recipient as Jarrad from Recent Recipient
  • Tap on Transaction Fee field
  • Tap on Reset to Default button
  • Tap on Done or on "x" to return to Send transaction screen
  • Check content of Choose Recipient field. Expected: it contains Jarrad as recipient

Questions:

  1. What is the difference between "x" and "Done" on Transaction Fee screen? Now both are closing the screen. Why to have both?
  2. If STT was selected as Asset on Send transaction screen then is it expected that Reset to Default resets defaults for ETH not STT? For example, I've selected STT and by default Gas Limit is 105000. If tap on Reset to Default then defaults for ETH are used, so Gas Limit becomes 21 000 and if return to Send Transaction screen then Asset is ETH instead of selected STT. My own expectation: becaue button Reset to Defaults resides on Transaction Fee screen it should affect only fields that can be edited on this screen. So, only Gas Limit and Gwei. If user selected STT or specified amount/recipient on other screen then these values should remain untouched.
  3. Do we expect to clean the content of Amount field on Send Transaction screen if Reset to Defaults is used On Transaction Fee screen where Amount is read-only? Should it be reset to empty or we should keep the value?

@jeluard
Copy link
Contributor Author

jeluard commented Feb 12, 2018

@annadanchenko I will fix your points #2 and #3 as they sound very similar to the first issue you are describing.
Question #1 is open to discussion.

@denis-sharypin
Copy link

@annadanchenko, @jeluard X button works more like Back button, it doesn't save changed parameters on the screen only Done button does.

@jeluard
Copy link
Contributor Author

jeluard commented Feb 12, 2018

@annadanchenko @denis-sharypin Made the change related to back button.

Signed-off-by: Julien Eluard <julien.eluard@gmail.com>
@jeluard jeluard merged commit ab5f1ae into develop Feb 13, 2018
@rasom rasom deleted the bug/#3213 branch April 19, 2018 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Reset to default and Done buttons are missing on Gas settings screen
6 participants