Tokens view #87

estebanmino · 2018-08-24T22:05:44Z

Description

This pull request adds the ability to handle assets (ERC20), adding manually custom tokens.

Checklist

Related #61

WIP

brunobar79

Honestly I'm impressed with the quality and speed that you made this. 🥇

I've added a few of comments mostly about the code style or some RN specifics, but otherwise looks great!

It would be great if you can add unit tests for this new components (not e2e yet because I didn't set it up for android)

brunobar79 · 2018-08-24T22:24:04Z

android/app/src/main/AndroidManifest.xml

@@ -25,7 +25,7 @@
        android:name=".MainActivity"
        android:label="@string/app_name"
        android:configChanges="keyboard|keyboardHidden|orientation|screenSize"
-        android:windowSoftInputMode="adjustResize"
+        android:windowSoftInputMode="adjustResize|stateUnspecified|adjustPan"


I don't think you need this if you wrap the main view in a scrollview instead of a view

brunobar79 · 2018-08-24T22:24:33Z

app/components/AddAsset/index.js

+
+	render() {
+		return (
+			<View style={styles.wrapper} testID={'add-asset-screen'}>


if you change this one for a scroll view you should be able to undo the changes you did on the manifest

I finally added a scrollview inside this, seems to work fine.

unless my idea was different

Don't we also need to make this View a ScrollView as well as @brunobar79 suggested? That way both AddAsset and AddCustomAsset can scroll properly.

there is a ScrollView inside it, now I changed it for ActionView

sorry I didn't change this for ActionView, it's only a View as you said. What do you think @brunobar79 ?

You added the scrollviews internally on each tab. That's ok with me

brunobar79 · 2018-08-24T22:25:37Z

app/components/AddAsset/index.js

+export default class AddAsset extends Component {
+	constructor(props) {
+		super(props);
+		this.state = {


move the state out of the constructor and remove the constructor (See other components)

I'll try it

brunobar79 · 2018-08-24T22:28:31Z

app/components/AddCustomAsset/index.js

@@ -0,0 +1,166 @@
+import React, { Component } from 'react';
+import { Text, TextInput, View, StyleSheet, TouchableOpacity } from 'react-native';


Try to avoid using TouchableOpacity and use Button from 'react-native-button'. The main reason is that for android Buttons should be TouchableNativeFeedback. The button components renders the right one depending on the OS.

brunobar79 · 2018-08-24T22:29:19Z

app/components/AddCustomAsset/index.js

+export default class AddCustomAsset extends Component {
+	constructor(props) {
+		super(props);
+		this.state = {


Same, move state out and remove the constructor

brunobar79 · 2018-08-24T22:37:45Z

app/components/SearchAsset/index.js

+ * View that provides ability to add custom assets.
+ */
+export default class SearchAsset extends Component {
+	constructor(props) {


same comment about removing constructor

brunobar79 · 2018-08-24T22:40:28Z

app/components/SearchAsset/index.js

+	};
+
+	cancelAddToken = () => {
+		this.props.navigation.push('Wallet');


same as before. pop or goBack

brunobar79 · 2018-08-24T22:40:38Z

app/components/SearchAsset/index.js

+				/>
+
+				<View style={styles.footer}>
+					<TouchableOpacity style={[styles.buttonCancel, styles.button]} onPress={this.cancelAddToken}>


brunobar79 · 2018-08-24T22:40:45Z

app/components/SearchAsset/index.js

+					<TouchableOpacity style={[styles.buttonCancel, styles.button]} onPress={this.cancelAddToken}>
+						<Text>CANCEL</Text>
+					</TouchableOpacity>
+					<TouchableOpacity style={[styles.buttonAddToken, styles.button]}>


brunobar79 · 2018-08-24T22:41:07Z

app/components/Wallet/index.js

 /**
- * Main view for the wallet
+ * Main view for the walletselectedAddress


walletselectedAddress is not a thing

brunobar79 · 2018-08-27T17:18:24Z

@estebanmino Now that I see the gif, Do you think you can validate the fields on the onBlur event? That way we don't show an error while you're still typing.

estebanmino · 2018-08-27T20:44:02Z

app/components/PageFooter/index.js

+		/**
+		 * Name of the current view
+		 */
+		onSubmit: PropTypes.object.function,


add comments

I approved and then I realized that we're still missing the unit tests

bitpshr

This code looks great and this is a very strong first PR against this project. Really great work. The code quality is top-notch, existing patterns were followed, and things work and work well.

I left a lot of nit-picky comments only to be thorough and to help you quickly identify what should be fixed. Every comment is minor, so don't let the number of comments discourage you. This is great stuff. Also, some general comments:

All components should at least have a snapshot test.
Any component that shows text should have a class applied with at least a fontStyle so the correct font family is applied. This includes inputs, <Text> components, etc.
We should create a ticket to finish the remainder of token adding, like searching for tokens.

bitpshr · 2018-08-30T03:52:52Z

app/components/AddAsset/index.js

+		decimals: ''
+	};
+
+	static navigationOptions = {


Nit: Can we put static properties above non-static properties like state?

bitpshr · 2018-08-30T03:54:35Z

app/components/AddAsset/index.js

+
+	render() {
+		return (
+			<View style={styles.wrapper} testID={'add-asset-screen'}>


Don't we also need to make this View a ScrollView as well as @brunobar79 suggested? That way both AddAsset and AddCustomAsset can scroll properly.

bitpshr · 2018-08-30T03:57:35Z

app/components/AddAsset/index.js

+		return (
+			<View style={styles.wrapper} testID={'add-asset-screen'}>
+				<ScrollableTabView renderTabBar={this.renderTabBar}>
+					<SearchAsset navigation={this.props.navigation} tabLabel="Search" />


Since renders are so expensive on React Native, it's important to keep them as optimal as possible. You can destructure props to save indirection of touching this.props.<some_prop> each time. For example, you could do const { navigation } = this.props; then use navigation directly without additional lookups to this and props.

It's a good idea to destructure as often as possible.

bitpshr · 2018-08-30T03:58:13Z

app/components/AddCustomAsset/index.js

+import { colors, fontStyles } from '../../styles/common';
+import Engine from '../../core/Engine';
+import PropTypes from 'prop-types';
+const isValidAddress = require('ethereumjs-util').isValidAddress;


You can change this to import { isValidAddress } from 'ethereumjs-util'; to stay consistent, it will still work.

bitpshr · 2018-08-30T03:59:06Z

app/components/AddCustomAsset/index.js

+	};
+
+	static propTypes = {
+		/**


This documentation formatting is off. It should be:

/** * navigation object required to push new views */

bitpshr · 2018-08-30T04:06:49Z

app/components/AddCustomAsset/index.js

+		let validated = true;
+		const decimals = this.state.decimals;
+		if (decimals.length === 0) {
+			this.setState({ warningDecimals: `Token precision can't be empty.` });


This string should use i18n.

bitpshr · 2018-08-30T04:07:10Z

app/components/AddCustomAsset/index.js

+		return (
+			<View style={styles.wrapper} testID={'add-custom-token-screen'}>
+				<ScrollView contentContainerStyle={styles.scrollViewWrapper}>
+					<Text>Token Address</Text>


This should mix in a fontStyle so it gets the correct font family. It should also use i18n.

bitpshr · 2018-08-30T04:07:48Z

app/components/AddCustomAsset/index.js

+					/>
+					<Text style={styles.warningText}>{this.state.warningAddress}</Text>
+
+					<Text>Token Symbol</Text>


This should mix in a fontStyle so it gets the correct font family. It should also use i18n.

bitpshr · 2018-08-30T04:08:06Z

app/components/AddCustomAsset/index.js

+				<PageFooter
+					onCancel={this.cancelAddToken}
+					onSubmit={this.addToken}
+					cancelText={'CANCEL'}


This (and submit below) should use i18n.

bitpshr · 2018-08-30T04:08:36Z

app/components/SearchAsset/index.js

+						onChangeText={this.onTokenChange}
+					/>
+				</ScrollView>
+				<PageFooter onCancel={this.cancelAddToken} cancelText={'CANCEL'} submitText={'NEXT'} />


These button texts should use i18n.

brunobar79 · 2018-08-31T02:13:32Z

locales/en.json

@@ -37,6 +37,25 @@
 		"no_tokens": "You don't have any tokens!",
 		"add_collectibles": "ADD COLLECTIBLES"
 	},
+	"addAsset": {
+		"title": "Add Asset",
+		"search_token": "Search",


Can you put this one and the one below all uppercase?

also titles? I saw other titles as Wallet and Transfer that are in the same format (lower case).

brunobar79

👍 After the last round of comments

bitpshr

Looks great! Very strong work.

Bump our `eslint` config dependencies.

* wallet get tokens from gaba * wallet add custom token view * fix animate footer above keyboard on android * wallet add asset scrollable search and custom token * wallet adding custom tokens * wallet add custom token validation * fix add asset touchable margin for many assets on list * wallet components delete constructors * wallet navigation pop for go back * wallet add custom token validate by input * wallet add custom token placeholders * wallet tokens using navigation goBack and validations onBlur * wallet using button instead of TouchableOpacity * wallet page footer component * wallet views android working with adjust resize * wallet tokens updating when added custom token * Update GABA version * wallet add custom token update tests * wallet add tokens consolidation * wallet add tokens update snapshots * wallet add tokens more consolidation * wallet add tokens consolidation * wallet add asset testIDs and localization update * wallet add asset style formatting

estebanmino added 7 commits August 24, 2018 13:18

wallet get tokens from gaba

f56791a

wallet add custom token view

5a484dc

fix animate footer above keyboard on android

c0ff4cc

wallet add asset scrollable search and custom token

6f42aef

wallet adding custom tokens

3661808

wallet add custom token validation

723c79a

fix add asset touchable margin for many assets on list

b2cf97a

brunobar79 suggested changes Aug 24, 2018

View reviewed changes

estebanmino added 4 commits August 24, 2018 20:42

wallet components delete constructors

7c80e08

wallet navigation pop for go back

dd8332b

wallet add custom token validate by input

3a15880

wallet add custom token placeholders

466c7dc

estebanmino added 3 commits August 27, 2018 16:11

wallet tokens using navigation goBack and validations onBlur

82b403d

wallet using button instead of TouchableOpacity

829d444

wallet page footer component

f53130a

estebanmino commented Aug 27, 2018

View reviewed changes

wallet views android working with adjust resize

14b2aec

brunobar79 previously approved these changes Aug 28, 2018

View reviewed changes

estebanmino and others added 2 commits August 28, 2018 20:04

wallet tokens updating when added custom token

1c43d97

Update GABA version

a1ce8ab

bitpshr force-pushed the tokens-view branch from df67359 to a1ce8ab Compare August 29, 2018 19:18

bitpshr suggested changes Aug 30, 2018

View reviewed changes

estebanmino added 6 commits August 30, 2018 18:17

wallet add custom token update tests

d2f5558

fix merge master conflicts

7e07f69

wallet add tokens consolidation

a79319f

wallet add tokens update snapshots

290e790

wallet add tokens more consolidation

f9ad687

wallet add tokens consolidation

8ddc983

estebanmino added 2 commits August 30, 2018 21:00

resolve merge master conflicts

974e132

wallet add asset testIDs and localization update

5918a39

brunobar79 reviewed Aug 31, 2018

View reviewed changes

brunobar79 approved these changes Aug 31, 2018

View reviewed changes

bitpshr approved these changes Aug 31, 2018

View reviewed changes

wallet add asset style formatting

32a0353

estebanmino merged commit 137d74a into master Aug 31, 2018

estebanmino deleted the tokens-view branch August 31, 2018 15:39

estebanmino mentioned this pull request Sep 5, 2018

Test e2e wallet add custom token #119

Merged

3 tasks

brunobar79 mentioned this pull request Nov 19, 2018

Tokens view #61

Closed

sethkfman pushed a commit that referenced this pull request Sep 3, 2021

@metamask/eslint-config*@8.0.0 (#87)

12e0e1a

Bump our `eslint` config dependencies.

		@@ -0,0 +1,166 @@
		import React, { Component } from 'react';
		import { Text, TextInput, View, StyleSheet, TouchableOpacity } from 'react-native';

Tokens view #87

Tokens view #87

Conversation

estebanmino commented Aug 24, 2018 • edited

brunobar79 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

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

brunobar79 commented Aug 27, 2018 • edited

Choose a reason for hiding this comment

bitpshr left a comment • edited

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

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

brunobar79 left a comment

Choose a reason for hiding this comment

bitpshr left a comment

Choose a reason for hiding this comment

estebanmino commented Aug 24, 2018 •

edited

brunobar79 commented Aug 27, 2018 •

edited

bitpshr left a comment •

edited