-
Notifications
You must be signed in to change notification settings - Fork 93
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
fix: autocomplete-onfocusout #855
Conversation
@@ -402,6 +402,14 @@ class AutoComplete extends Component<IAutoCompleteProps, IAutoCompleteState> { | |||
return max; | |||
} | |||
|
|||
onFocusOut = (event: any) => { |
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.
What do you think of renaming this function to closeSuggestionDialog
?
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.
Its purpose isn't to close the dialog. It's to monitor the focusOut event of the div and its children. closing the dialog could be the name of the function that closes the dialog and reused however since it's already a one-liner implementation that is explanatory, I do not exactly know the effect
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.
What happens when the function is called from a user's perspective? Also from a readability perspective onBlur={closeSuggestionDialog}
reads better than onBlur={onFocusOut}
you could rename onFocusOut
to removeFocus
so that it reads onBlur={removeFocus}
but that won't communicate the intent as clearly as onBlur={closeSuggestionDialog}
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 see what you mean. What do you think of handleClickOutside
? It encapsulates that we are handling a user's activity when they click outside. The reader may be misinformed by closeSuggestionDialog
since they'd expect that this function is also called when someone selects a suggestion
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.
@thewahome handleClickOutside
does not communicate the intent because it doesn't clearly answer the question what does this function do? Can we rename the function that handles selecting a suggestion to selectSuggestion
so as to avoid the ambiguity and then it will read onClick={selectSuggestion}
and onBlur={closeSuggestionDialog}
There's an issue with regard to accessibility here, a user navigating the site using the Tab key won't be able to write a query or even edit one in the query input field. |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-855.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-855.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-855.centralus.azurestaticapps.net |
Overview
Autocomplete suggestions should collapse when the autocomplete component loses focus
Fixes #721
Demo
Optional. Screenshots,
curl
examples, etc.Notes
OnFocusOut is not supported in React hence the use of onBlur.
Testing Instructions