[Labs] Listen to query on select components #1353
Conversation
Listen to query on select componentsPreview: documentation |
| const query = e.currentTarget.value; | ||
| this.setState({ query, isOpen: !(query.length === 0 && this.props.openOnKeyDown) }); | ||
| this.setState({ query, isOpen: !(query.length === 0 && openOnKeyDown) }); |
There was a problem hiding this comment.
This is a little tricky to parse mentally. I'd vote for query.length > 0 || !openOnKeyDown).
There was a problem hiding this comment.
Also unrelated, but happy to make the change
| @@ -172,9 +173,9 @@ export class Omnibox<T> extends React.Component<IOmniboxProps<T>, IOmniboxState< | |||
| private maybeRenderMenu(listProps: IQueryListRendererProps<T>) { | |||
| if (this.state.query.length > 0) { | |||
| return ( | |||
| <ul className={CoreClasses.MENU} ref={listProps.itemsParentRef}> | |||
| <Menu ulRef={listProps.itemsParentRef}> | |||
There was a problem hiding this comment.
Is this related to the onChange changes, or is this unrelated?
There was a problem hiding this comment.
Unrelated, I apparently forgot to do that in a previous PR
Tweak query change predicatePreview: documentation |
| {...tagInputProps} | ||
| inputProps={defaultInputProps} |
There was a problem hiding this comment.
moving this line does not feel correct as it prevents changing any of the fields defined in defaultInputProps. why did you do this?
There was a problem hiding this comment.
Ha, you're absolutely right.
| const query = e.currentTarget.value; | ||
| this.setState({ query, isOpen: !(query.length === 0 && this.props.openOnKeyDown) }); | ||
| this.setState({ query, isOpen: query.length > 0 || !openOnKeyDown }); | ||
| Utils.safeInvoke(tagInputProps.inputProps.onChange, e); |
There was a problem hiding this comment.
❌ not safe yet, if tagInputProps is {} then inputProps is undefined => error.
gotta add a check that tagInputProps.inputProps exists.
| @@ -248,8 +248,9 @@ export class Select<T> extends React.Component<ISelectProps<T>, ISelectState<T>> | |||
| Utils.safeInvoke(popoverProps.popoverWillClose); | |||
| } | |||
|
|
|||
| private handleQueryChange = (event: React.FormEvent<HTMLInputElement>) => { | |||
| this.setState({ query: event.currentTarget.value }); | |||
| private handleQueryChange = (e: React.FormEvent<HTMLInputElement>) => { | |||
There was a problem hiding this comment.
prefer calling it event, like it was. (see changes above, where param is still called event)
There was a problem hiding this comment.
Ha, was just trying to stay consistent with previous feedback from @adidahiya: #1275 (comment)
What do you think? Happy to change back to event across labs, let me know.
There was a problem hiding this comment.
e makes sense for the no-conflict window.event argument. all i care about is consitency
Check that inputProps is definedPreview: documentation |
restProps, e -> eventPreview: documentation |
| @@ -190,6 +199,7 @@ export class Omnibox<T> extends React.Component<IOmniboxProps<T>, IOmniboxState< | |||
|
|
|||
| private handleQueryChange = (event: React.FormEvent<HTMLInputElement>) => { | |||
| this.setState({ query: event.currentTarget.value }); | |||
| Utils.safeInvoke(this.props.inputProps.onChange, event); | |||
There was a problem hiding this comment.
gotta make sure inputProps exists before you reference it here. or do the { inputProps = {} } deconstruction
There was a problem hiding this comment.
Ha yeah, I always forget. Would be nice to be able to set defaultProps on those generic components!
| @@ -250,6 +258,7 @@ export class Select<T> extends React.Component<ISelectProps<T>, ISelectState<T>> | |||
|
|
|||
| private handleQueryChange = (event: React.FormEvent<HTMLInputElement>) => { | |||
| this.setState({ query: event.currentTarget.value }); | |||
| Utils.safeInvoke(this.props.inputProps.onChange, event); | |||
There was a problem hiding this comment.
again must make sure inputProps exists before dereferencing it.
Check existence of inputPropsPreview: documentation | landing | table |
|
@cmslewis ready for review again when you get a chance |
inputProps.onChangeto listen to the query without controllinginputProps.value