Sanitize ChatInviteDialog #626
Conversation
* Use binds rather than onFoo functions which aren't actually handler functions themselves but return them * Rename onKeyUp to moveSelectionDown etc,, reserving onKeyUp for "a key has been released" rather than, "the up arrow key has been pressed"
* Pressing enter now always adds whatever was in the input box to the invite list, if it's a valid address (previously it added it to the list of it was a search result but submitted the form straight away if there were no results). * Remove isValidAddress as it was only used in the context of testing whether its return value was true or null (where null meant "unsure") so just use getAddressType instead.
| @@ -124,7 +118,7 @@ module.exports = React.createClass({ | |||
| // Saving the addressListElement so we can use it to work out, in the componentDidUpdate | |||
| // method, how far to scroll when using the arrow keys | |||
| addressList.push( | |||
| <div className={classes} onClick={this.onClick(i)} onMouseEnter={this.onMouseEnter(i)} onMouseLeave={this.onMouseLeave} key={i} ref={(ref) => { this.addressListElement = ref; }} > | |||
| <div className={classes} onClick={this.onClick.bind(this, i)} onMouseEnter={this.onMouseEnter.bind(this, i)} onMouseLeave={this.onMouseLeave} key={i} ref={(ref) => { this.addressListElement = ref; }} > | |||
There was a problem hiding this comment.
this is fine, but for future reference:
(i) => {this.onClick(i)} is preferable to this.onClick.bind(this,i)
A separate AddressListEntry component, with its own onClick etc methods, is more reacty and doesn't require creating new closures on each render cycle. Note that such a component doesn't even need to be in a different file.
| @@ -135,12 +135,26 @@ module.exports = React.createClass({ | |||
| } else if (e.keyCode === 13) { // enter | |||
| e.stopPropagation(); | |||
| e.preventDefault(); | |||
| this.onButtonClick(); | |||
| if (this.state.queryList.length > 0) { | |||
| this.addressSelector.chooseSelection(); | |||
There was a problem hiding this comment.
this code will never be reached - it is handled at line 127.
| if (this.state.queryList.length > 0) { | ||
| this.addressSelector.chooseSelection(); | ||
| } else { | ||
| const addrType = Invite.getAddressType(this.refs.textinput.value); |
There was a problem hiding this comment.
let's factor this out to save duplicating it between here and comma/tab
There was a problem hiding this comment.
... and so that we can give it a name. "addInputToList" ?
| @@ -135,12 +135,26 @@ module.exports = React.createClass({ | |||
| } else if (e.keyCode === 13) { // enter | |||
| e.stopPropagation(); | |||
| e.preventDefault(); | |||
| this.onButtonClick(); | |||
There was a problem hiding this comment.
so previously, we submitted the form if the input box was empty. I think that is desirable behaviour. Can we keep it?
if input is empty
And fix typo where it was handled
| return inviteList; | ||
| } else { | ||
| this.setState({ error: true }); | ||
| return false; |
There was a problem hiding this comment.
bit weird to return Boolean or Array? null would make a more conventional empty mark?
Two commits. The first cleans up the code a little with no behavioural changes. The second changes what happens when you press 'enter', making it consistently add the thing in the text box to the list of addresses to invite (if it's a valid address) rather than sometimes doing that and sometimes submitting the form straight away.