Transfer #224

Merged
merged 20 commits into from Jan 10, 2017

Projects

None yet

3 participants

@adcpm
Owner
adcpm commented Jan 7, 2017 edited
  • Create page /transfer
  • Add Coinmarketcap API to show Steem rate
@adcpm adcpm temporarily deployed to busy6-pr-224 Jan 7, 2017 Inactive
@adcpm adcpm temporarily deployed to busy6-pr-224 Jan 8, 2017 Inactive
@adcpm adcpm temporarily deployed to busy6-pr-224 Jan 8, 2017 Inactive
@adcpm adcpm temporarily deployed to busy6-pr-224 Jan 8, 2017 Inactive
@adcpm adcpm temporarily deployed to busy6-pr-224 Jan 9, 2017 Inactive
@adcpm adcpm temporarily deployed to busy6-pr-224 Jan 9, 2017 Inactive
src/app/Sidebar.js
@@ -3,19 +3,18 @@ import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import { Link } from 'react-router';
import { FormattedMessage } from 'react-intl';
+import fetch from 'isomorphic-fetch';
@nil1511
nil1511 Jan 9, 2017 Collaborator

missing in package.json

src/app/Sidebar.js
+
+ fetch('https://api.coinmarketcap.com/v1/ticker/steem/')
+ .then(res =>
+ res.json().then((json) => {
@nil1511
nil1511 Jan 9, 2017 edited Collaborator

Writing as would be better.

fetch('https://api.coinmarketcap.com/v1/ticker/steem/')
      .then(res => res.json())
      .then((json) => {
        const priceUsd = json[0].price_usd;
        this.setState({ price: priceUsd });
      });

Also it would better to save price in redux state.

src/app/Sidebar.js
- var base = (this.state.feedPrice.base).replace(' SBD', '').replace(',', '');
- var dollar = (parseFloat(base) * (parseFloat(user.balance) + parseFloat(power))) + parseFloat(user.sbd_balance);
- }
+ const power = props
@nil1511
nil1511 Jan 9, 2017 Collaborator

(props) this.state.props is confusing would be nice to name as categoriesProp(if it is).

src/wallet/Transfer.js
+ auth: state.auth,
+ })
+)
+
@nil1511
nil1511 Jan 9, 2017 Collaborator

Remove the space.

@adcpm
adcpm Jan 9, 2017 Owner

Is there any reason to not having a space between connect and the class?

@p0o
p0o Jan 9, 2017 Collaborator

@adcpm connect is a Higher Order Component which means is containing the whole class like a function. As we don't leave a space in this example:

function something(

{ foo: 'bar' }
) { ...

IMHO we also shouldn't leave there because empty lines are used for separating conceptually independent parts but connect and the class it's containing are tightly coupled.

src/wallet/Transfer.js
+ <div className="form-group">
+ <a
+ href={url}
+ target="_blank"
@nil1511
nil1511 Jan 9, 2017 Collaborator

security tip

always add rel="noopener noreferrer" with target="_blank" if url is leaving the website.

@p0o
p0o Jan 9, 2017 Collaborator

@nil1511 why is that? hiding track of staging URL?

@nil1511
nil1511 Jan 9, 2017 edited Collaborator

by default new url expose window.opener pointing to previous page. noopener will set it to null and noreferrer removes referrer information.

@adcpm
adcpm Jan 9, 2017 Owner

That's a linter rule

@p0o
p0o Jan 10, 2017 Collaborator

@nil1511 I just read about it. window.opener is a crazy API! thank you

@adcpm adcpm temporarily deployed to busy6-pr-224 Jan 9, 2017 Inactive
@adcpm adcpm temporarily deployed to busy6-pr-224 Jan 9, 2017 Inactive
@adcpm
Owner
adcpm commented Jan 9, 2017

I've done the requested changes @nil1511 @p0o

@p0o
p0o approved these changes Jan 10, 2017 View changes
@nil1511 nil1511 merged commit f3cab8d into dev Jan 10, 2017
@nil1511 nil1511 deleted the transfer branch Jan 10, 2017
@nil1511 nil1511 removed the waiting-review label Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment