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

[Wallet] Add support for address pasting in send input field #1180

Merged
merged 7 commits into from
Oct 2, 2019

Conversation

jmrossy
Copy link
Contributor

@jmrossy jmrossy commented Oct 2, 2019

Description

  • Add support for detecting addresses in a clipboard
  • Fix pasting into send field
  • Improve modularity and typing of text input components

Tested

Loaded send with and without address in clipboard
Ran through send flow
Ran through invite flow

Related issues

Backwards compatibility

Yes

wa-send-paste
wa-send-paste2

@@ -10,6 +10,8 @@
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
<uses-permission android:name="android.permission.CAMERA" />

<uses-sdk android:minSdkVersion="16" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this back, I mistakenly removed the line in my last PR but should have just removed the targetSDK field in it

@@ -0,0 +1,88 @@
// HOC to add a label (e.g. an icon or a currency code) to a text input
Copy link
Contributor Author

@jmrossy jmrossy Oct 2, 2019

Choose a reason for hiding this comment

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

FYI, this is mostly based on LabeledTextInput.tsx which was deleted

@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #1180 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
+ Coverage   66.16%   66.16%   +<.01%     
==========================================
  Files         262      261       -1     
  Lines        7610     7599      -11     
  Branches      443      440       -3     
==========================================
- Hits         5035     5028       -7     
+ Misses       2478     2476       -2     
+ Partials       97       95       -2
Flag Coverage Δ
#mobile 66.16% <100%> (ø) ⬆️
Impacted Files Coverage Δ
packages/mobile/src/recipients/RecipientPicker.tsx 73.07% <100%> (+1.45%) ⬆️
packages/mobile/src/send/SendAmount.tsx 77.77% <100%> (+0.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe56034...11accfa. Read the comment docs.

Copy link
Contributor

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Looks good!

Though I understand the idea to separate the different TextInput features (paste and labeling), I'm a bit wary of the usage of HOC for this, I find it makes it non obvious to understand how to use the HOCs.
What would happen if they aren't composed in the right order?
I might be missing something but having props to enable/disable labels and paste would seem simpler as a user of TextInput.

render() {
const { isPasteIconVisible } = this.state

// TODO(Rossy) Use a more paste-y instead of copy looking icon when we have one
Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly what I thought when I saw the icon 😄

Comment on lines +40 to +42
this.setState({ isPasteIconVisible: true })
} else {
this.setState({ isPasteIconVisible: false })
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not really a problem here but this style of async usage could make it call setState after the component has been unmounted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a general strategy on how to avoid that?

@jmrossy jmrossy merged commit 1522211 into master Oct 2, 2019
@jmrossy jmrossy deleted the rossy/wa-input-paste branch October 2, 2019 20:24
aaronmgdr added a commit that referenced this pull request Oct 4, 2019
* master:
  [Protocol] Fix network id for alfajores in truffle configs (#1211)
  When resetting and upgrading a VM testnet, new tx-nodes are included in the new instance group (#771)
  Upload static VM testnet nodes, add stackdriver logging (#750)
  Revert "Make packages depend on git vesrion (not npm)" (#1201)
  Make packages depend on git vesrion (not npm) (#1192)
  [contractkit] Document methods (#1195)
  [ck] consistent send tx object in kit (#1191)
  Move docker images to use node v10 (#1183)
  [ContractKit]Fill more fields before web3 signing (#1133)
  [codecov]Fix codecov errors (#1147)
  [Wallet] Add support for address pasting in send input field (#1180)
  Fix verification pool validation (#1176)
  Improve QR Code scan ability (#1036)
  Add CLI commands around identity metadata (#1167)
  [wallet]Run geth in an infura-like mode (#1108)

# Conflicts:
#	yarn.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send screen input should allow users to paste
5 participants