Skip to content
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

Merged
merged 4 commits into from
Feb 23, 2021
Merged

Conversation

thewahome
Copy link
Collaborator

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

  • Trigger an autocomplete call
  • Once the options show up click outside the component
  • Notice that the suggestions close

@@ -402,6 +402,14 @@ class AutoComplete extends Component<IAutoCompleteProps, IAutoCompleteState> {
return max;
}

onFocusOut = (event: any) => {
Copy link
Contributor

@jobala jobala Feb 19, 2021

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?

Copy link
Collaborator Author

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

Copy link
Contributor

@jobala jobala Feb 19, 2021

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}

Copy link
Collaborator Author

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

Copy link
Contributor

@jobala jobala Feb 19, 2021

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}

@ElinorW
Copy link
Collaborator

ElinorW commented Feb 19, 2021

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.
It's inaccessible using the Tab key.

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-855.centralus.azurestaticapps.net

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-855.centralus.azurestaticapps.net

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-855.centralus.azurestaticapps.net

@thewahome thewahome merged commit e80af0b into dev Feb 23, 2021
thewahome added a commit that referenced this pull request Mar 3, 2021
* Fix: autocomplete-onfocusout (#855)

* Feature: make paging easier (#861)

* Fix: metadata syntax highlighting (#864)

* Fix: Adaptive cards changes (#873)

* Fix: snippets not available (#877)
@thewahome thewahome deleted the fix/autocomplete-collapse-onFocusOut branch August 9, 2021 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete doesn't collapse onFocusOut
3 participants