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.

Sign up for GitHub

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

Jump to bottom

fix: autocomplete-onfocusout #855

Merged
merged 4 commits into from
Feb 23, 2021
Merged

fix: autocomplete-onfocusout #855

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5df9af2
close suggestions onFocusOut
thewahome Feb 18, 2021
e3640c7
fix accessibility problem introduced
thewahome Feb 23, 2021
6f20d29
rename functions for better readability
thewahome Feb 23, 2021
b1b55e9
remove aria variable affecting accessibility
thewahome Feb 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/app/views/query-runner/query-input/auto-complete/AutoComplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,14 @@ class AutoComplete extends Component<IAutoCompleteProps, IAutoCompleteState> {
return max;
}

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

@jobala jobala Feb 19, 2021 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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}

const { currentTarget, relatedTarget } = event;
if (!currentTarget.contains(relatedTarget as Node) && this.state.showSuggestions) {
this.setState({
showSuggestions: false
})
}
}

public render() {
const {
Expand All @@ -418,7 +426,7 @@ class AutoComplete extends Component<IAutoCompleteProps, IAutoCompleteState> {
}: any = queryInputStyles(currentTheme).autoComplete;

return (
<>
<div tabIndex={0} onBlur={this.onFocusOut}>
<TextField
className={autoInput}
type='text'
Expand All @@ -437,7 +445,7 @@ class AutoComplete extends Component<IAutoCompleteProps, IAutoCompleteState> {
filteredSuggestions={filteredSuggestions}
activeSuggestion={activeSuggestion}
onClick={(e: any) => this.onClick(e)} />}
</>
</div>
);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/app/views/query-runner/query-input/auto-complete/SuggestionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const SuggestionsList = (props: any) => {
{filteredSuggestions.map((suggestion: {} | null | undefined, index: number) => {
return (
<li
tabIndex={index}
className={(index === activeSuggestion) ? classes.suggestionActive : classes.suggestionOption}
key={index}
ref={refs[index]}
Expand Down