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

Make address selection searchable #2739

Merged
merged 3 commits into from
Oct 20, 2016
Merged

Make address selection searchable #2739

merged 3 commits into from
Oct 20, 2016

Conversation

ngotchac
Copy link
Contributor

Fixes #2141

Implements address selection in transaction modal with an autocomplete, so they can be searched through...

See it in action : https://youtu.be/C-xgoemIs7c

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M5-ui labels Oct 19, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 86.263% when pulling 0a39be6 on ng-searchable-addresses into 146cefd on master.

@@ -75,6 +93,13 @@ class InputAddress extends Component {
</div>
);
}

handleInputChange = (event, value) => {
let isEmpty = (value.length === 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

const? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, still some sticking old habits...

Copy link
Contributor

Choose a reason for hiding this comment

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

:D It happens.

}

onFocus = () => {
this.props.onChange(null, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Throw-away comment, i.e. not for the PR. Think we probably need to re-work the event handlers in a future generation to always only return the value everywhere. Currently it is a bit of a mix, some are value, some are event & value, some are event, index & value. (And event is never handled in the React components, always only in the inputs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, agreed. event should be only within Inputs wrappers

{ this.renderSelectEntries() }
</Select>
<div>
<AutoComplete
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we (for the sake of consistency in the input components), add the AutoComplete to where we have the other input components, i.e. a thin wrapper. It feels like the odd man out atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes that's right. That's what I initially planned, but forgot... Will do!

@jacogr
Copy link
Contributor

jacogr commented Oct 19, 2016

Small comments, minor issues. Fix and we can push it in.

@ngotchac ngotchac added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 19, 2016
@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 20, 2016
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 20, 2016
@jacogr jacogr merged commit 1e21b07 into master Oct 20, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 86.259% when pulling f618b36 on ng-searchable-addresses into 146cefd on master.

@jacogr jacogr deleted the ng-searchable-addresses branch October 20, 2016 12:26
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