Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Convert ShapeShift modal to store #4035

Merged
merged 17 commits into from
Jan 5, 2017
Merged

Convert ShapeShift modal to store #4035

merged 17 commits into from
Jan 5, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Jan 4, 2017

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Jan 4, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 4, 2017
Copy link
Contributor

@ngotchac ngotchac 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 about error handling. Looks good otherwise


if (error.fatal) {
if (error) {
return (
<ErrorStep error={ error } />
Copy link
Contributor

Choose a reason for hiding this comment

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

ErrorStep missing the store

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dang.

this.setPrice(price);
})
.catch((error) => {
console.error('getCoinPrice', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the error be handled here ? eg. select 'ETH' from the tokens select
It might be that the token is not available, thus the transfer shouldn't be available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never did, think it is a nice to have with unneeded complexity for the amount of use we get here atm. Doing so will introduce another error class, something that is not fatal and should just be displayed, call it a warning.

Might be slightly annoying but as it stands it doesn't allow you to do anything stupid, e.g. when you do shift using eth->eth it will show the error when trying to execute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, after a coffee walk, added a warning (along the same lines as what we have in other modals). It will still allow you to proceed in this case, obviously the attempt to shift will throw an error and display it as such.

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 5, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Jan 5, 2017

parity 2017-01-05 13-44-40

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 5, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5e0b116 on jg-shapeshift-store into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5e0b116 on jg-shapeshift-store into ** on master**.

@jacogr jacogr merged commit 9613145 into master Jan 5, 2017
@jacogr jacogr deleted the jg-shapeshift-store branch January 5, 2017 15:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants