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

(Feature) UI Improvements in Invest page #341

Merged
merged 6 commits into from
Nov 15, 2017
Merged

(Feature) UI Improvements in Invest page #341

merged 6 commits into from
Nov 15, 2017

Conversation

fvictorio
Copy link
Contributor

Depends on #199.

This PR builds upon #199 and adds some improvements:

  • Modify the method signature. Use 0xa6f2ae3a, that corresponds to buy()
  • Add QR with the address of the crowdsale
  • Disable MetaMask option if web3 is not injected
  • Hide Invest button when MetaMask is not selected

Copy link
Contributor

@mark-antony1 mark-antony1 left a comment

Choose a reason for hiding this comment

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

A few comments, but looks good to me otherwise.

window.scrollTo(0, 0);
if (this.tokensToInvestOnChange.bind) this.tokensToInvestOnChange = this.tokensToInvestOnChange.bind(this);
if (this.investToTokens.bind) this.investToTokens = this.investToTokens.bind(this);
var state = defaultState;
state.seconds = 0;
state.loading = true;
state.pristineTokenInput = true;
state.investThrough = 'metamask'
state.web3Available = false;
state.crowdsaleAddress = ICOConfig.crowdsaleContractURL || getURLParam("addr")
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract lines 21-26 to an object in a constant or util file and use Object.assign to add it to the default state on line 20

return
};

const networkID = ICOConfig.networkID?ICOConfig.networkID:getQueryVariable("networkID");
const contractType = this.state.contractTypes.whitelistwithcap;// getQueryVariable("contractType");
checkNetWorkByID(web3, networkID);
newState.contractType = contractType;
newState.web3Available = true;
newState.investThrough = 'metamask';
Copy link
Contributor

Choose a reason for hiding this comment

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

extract string to constant

<a className="button button_fill" onClick={this.investToTokens}>Invest now</a>
<div className="invest-through-container">
<select value={this.state.investThrough} className="invest-through" onChange={(e) => this.setState({ investThrough: e.target.value })}>
<option disabled={!this.state.web3Available} value="metamask">Metamask {!this.state.web3Available ? ' (not available)' : null}</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

extract ternary to variable

@fvictorio
Copy link
Contributor Author

Let's try to merge this as soon as possible, so that we can work on solving the MobX PR conflicts. I will create issues for all the non-critical stuff that is pending.

Copy link
Collaborator

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

  1. "Contribute" should be instead "Invest" for the button
  2. Wrong method signature "Send ethers to the crowdsale address with a MethodID: 0xf2fde38b"

@fvictorio
Copy link
Contributor Author

  1. Sorry about that, it got lost during the merge conflict resolution.
  2. I didn't get this. What do you mean? That hash is the signature of the buy() method, did that change?

@vbaranov
Copy link
Collaborator

  1. The methodID should be: 0xa6f2ae3a

screen shot 2017-11-15 at 18 43 26

@fvictorio
Copy link
Contributor Author

Fixed, thanks. Just curious, to what method corresponds that hash?

@vbaranov
Copy link
Collaborator

vbaranov commented Nov 15, 2017

0xa6f2ae3a is for buy method. Do you mean wrong hash? I guess, it was just a mock hash in invest-page-2 branch, because it was created long ago.

@vbaranov vbaranov self-requested a review November 15, 2017 16:38
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.

4 participants