-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
React/DP-13718: Fuzzy search default suggestions. #544
Conversation
… to actually pass useful information.
}); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change on purpose? moving the callback into setState in the if block will only callback if value is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we are not using state in props.onChange function, what's the difference putting it in callback vs outside of setState? if we pass it as callback we will have to do it in the setState below also
highlightedItemIndex: null | ||
}, () => { | ||
if (is.fn(this.props.onSuggestionClick)) { | ||
this.props.onSuggestionClick(event, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think we should pass this callback in blur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onSuggestionClick is when you select on a suggestion, e.g. we use it to navigate to a url and that shouldn't be triggered onBlur
onKeyDown: (event, { newHighlightedSectionIndex, newHighlightedItemIndex }) => { | ||
onFocus: this.handleFocus, | ||
onBlur: (event) => { | ||
event.persist(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think we need to persist this event unless we want to pass a custom callback
Minor
Added
renderDefaultSuggestion
Removed
To test:
yarn link-mayflower in budget: https://github.com/massgov/budget/pull/242