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

Fixed bool dropdown in contract execution #3823

Merged
merged 8 commits into from Dec 12, 2016
Merged

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 12, 2016

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 12, 2016
{ boolitems }
</Select>
);
}

onChangeBool = (event, _index, value) => {
this.props.onChange(value === 'true');
// NOTE: event.target.value added for enzyme simulated event testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Not happy with this but i'm not sure there's a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use the _index var ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ngotchac Isn't it kind of fragile to depend on the order in the dropdown?

Thinking about e.target.value: It's actually spec-compatible, so I'm fine with it. Material UI actually adds the value parameter in a non-spec way.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ['false', 'true'] Array should be saved and used when creating the menu items and onChange IMO

@derhuerst derhuerst added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 12, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3a7e30e on jg-typedinput-tests into ** on master**.

@gavofyork gavofyork merged commit 3a16247 into master Dec 12, 2016
@gavofyork gavofyork deleted the jg-typedinput-tests branch December 12, 2016 14:29
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

5 participants