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

Use TypedInputs in Contracts view #4015

Merged
merged 4 commits into from
Jan 3, 2017
Merged

Use TypedInputs in Contracts view #4015

merged 4 commits into from
Jan 3, 2017

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Jan 3, 2017

Closes #3918

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 3, 2017
@@ -78,7 +78,7 @@ class InputAddress extends Component {
return (
<div className={ containerClasses.join(' ') }>
<Input
allowCopy={ allowCopy && (disabled ? value : false) }
allowCopy={ allowCopy && ((disabled || readOnly) ? value : false) }
Copy link
Contributor

Choose a reason for hiding this comment

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

allowCopy && !disabled && !readOnly && value?

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 want it to be false if value is falsy, because otherwise (if allowCopy isn't a boolean) the copy button would show and copy the passed value (could be '')

this.setState({ isEth: true, ethValue: fromWei(this.props.value) });
const { isEth, value } = this.props;

if (typeof isEth === 'boolean' && value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is a specific reason for this, but isn't boolean validation done by propTypes already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be undefined I think

Copy link
Contributor

Choose a reason for hiding this comment

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

undefined is falsy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the logic is :

  • If isEth is a prop (false or true), then the ETH toggle will be shown for integer inputs
    The prop value will then be the default toggle state (ie.false doesn't convert the value to ETH by default)
  • Else (ie. undefined, not a boolean), don't show the toggle at all

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then my only grumble is the tri-state isEth (null is the default value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it's more or less like allowCopy : could be true or false whether to show it or not, or the value to actually copy. I agree that it might not be the best, but I'm not sure that it's in the scope of this PR.

label={ label }
hint={ hint }
value={ realValue }
error={ error }
onChange={ onChange }
type='number'
readOnly={ readOnly }
type={ readOnly ? 'text' : 'number' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because if the TypedInput is read only and a number, we display the formatted number (which must be in a text input then)

const param = this.getParam();

const realValue = value && typeof value.toNumber === 'function'
? value.toNumber()
? (readOnly ? value.toFormat() : value.toNumber())
Copy link
Contributor

@derhuerst derhuerst Jan 3, 2017

Choose a reason for hiding this comment

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

[tiny grumble] I'd rather check if it's a BigNumber here, because you use both toFormat & toNumber.

label={ label }
hint={ hint }
value={ value }
editing
Copy link
Contributor

Choose a reason for hiding this comment

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

afaict editing does not exist as a prop.

} else if (api.util.isArray(value)) {
}

if (api.util.isArray(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something is returned in the previous if statement, thus no need for else

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 3, 2017
@jacogr jacogr merged commit e8ef7b3 into master Jan 3, 2017
@jacogr jacogr deleted the ng-advanced-contracts branch January 3, 2017 16:41
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.

3 participants