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

Account selector close operations #4728

Merged
merged 13 commits into from
Mar 3, 2017
Merged

Account selector close operations #4728

merged 13 commits into from
Mar 3, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Mar 2, 2017

  • Close when clicking anywhere in the body pane
  • Close when clicking the address (previously only focusses)
  • Close when pasting a valid address
  • Fix invalid proptypes handling in InputAddressSelect
  • Filtering, closing & re-opening now clears the filter (Thanks @ngotchac)

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Mar 2, 2017
@@ -613,6 +617,10 @@ class AddressSelect extends Component {
focusedItem: null,
inputValue: value
});

if (apiutil.isAddressValid(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not importing directly isAddressValid (I think that's how it's done pretty much everywhere)

Copy link
Contributor Author

@jacogr jacogr Mar 3, 2017

Choose a reason for hiding this comment

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

Not sure it is "the default" - have not done it once myself. (And I do prefer the above format to all the direct function imports - can see exactly where things are from. It gets messy at some point when you have a large number - obviously not the case here)

Copy link
Contributor

Choose a reason for hiding this comment

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

Quite true, thought so. It's imported twice as apiutil and once as util (for use with isAddressValid) though

@@ -28,6 +28,7 @@ import styles from './accountCard.css';
export default class AccountCard extends Component {
static propTypes = {
account: PropTypes.object.isRequired,
allowAddressClick: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's not very common practice, but could we add static defaultProps with this allowAddressClick set to false ? I think it would be a bit more obvious what the default behavior is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, like being explicit, adding it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

(It's defaultProps not defaultPropTypes)

Copy link
Contributor Author

@jacogr jacogr Mar 3, 2017

Choose a reason for hiding this comment

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

sigh Sorry, I setup my auto-complete wrongly. (Happens when you do it late at night.) Updating. (Edit: thanks for the fix)

@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 Mar 3, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Mar 3, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Mar 3, 2017

Ok, so I fixed to filter issue (not reset filter on close), and prevented the default click & select action when selecting text. It seems that there still are some parts of the pop-up where clicking doesn't do anything. Not sure if that's wanted or not

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 3, 2017
@jacogr jacogr merged commit 6760ae0 into master Mar 3, 2017
@jacogr jacogr deleted the jg-selector-close branch March 3, 2017 13:38
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.

2 participants