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

Add new behavior to autocomplete component #85

Merged

Conversation

trevorhoneycutt
Copy link

@trevorhoneycutt trevorhoneycutt commented Apr 26, 2024

This PR adds fillOnSelected and blurOnSelect props to the autocomplete component.

  1. fillOnSelected: Controls if the input field is automatically filled with the selected suggestion.
  2. blurOnSelect: Determines whether the input field should lose focus after a selection.
  3. Clicking tab does not dismiss the suggestion list.

@trevorhoneycutt trevorhoneycutt changed the title Add new props to Autocomplete Add new behavior to autocomplete component Apr 26, 2024
@@ -218,6 +227,7 @@
this.activated = false;
},
handleKeyEnter(e) {
if (this.blurOnSelect) { this.$refs.input.ignoreNextBlur(false); }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Info since the mouse cursor never click when using the keyboard to make the selection, the input is never unfocused. Thus, we do not need to ignore the next blur event.

@@ -189,6 +197,7 @@
},
handleInput(value) {
this.$emit('input', value);
if (this.blurOnSelect) { this.$refs.input.ignoreNextBlur(true); }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I crazy or is the meaning of this prop backwards? To me this reads as "If blur on select, then don't blur when a value is selected". Am I missing something or should it be negated?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not crazy. I had everything backwards on accident.

Trevor Honeycutt added 2 commits April 29, 2024 16:33
@@ -208,6 +219,7 @@
}
},
handleBlur(event) {
this.activated = false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this.activated = false; for?

@@ -230,7 +247,8 @@
}
},
select(item) {
this.$emit('input', item[this.valueKey]);
this.selected = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does selected mean that you're in the process of selecting an item (but not yet selected)? If so that might be a better form of name to avoid confusing this with a "currently selected value", which is a common data property.

@@ -216,8 +228,13 @@
},
close(e) {
this.activated = false;
if (!this.selected && !this.blurOnSelect) {
this.selected = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is selected set to true here?

Copy link
Author

@trevorhoneycutt trevorhoneycutt Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set selected=true so that this won't conditional will not run again

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the quick chat yesterday, this is really something like "was select handled", so you're saying that the selection was handled at this point (and the field was blurred)? But, I think my confusion comes from assuming that when the autocomplete it closed this prop wouldn't be checked again until it was opened again, and it should go back to its default state (false).

Trevor Honeycutt added 2 commits April 30, 2024 16:26
@@ -18,7 +18,7 @@
@keydown.up.native.prevent="highlight(highlightedIndex - 1)"
@keydown.down.native.prevent="highlight(highlightedIndex + 1)"
@keydown.enter.native="handleKeyEnter"
@keydown.native.tab="close"
@keydown.native.tab="handleKeyEnter"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an intended functional change, i.e. tab to select the currently highlighted value? And if so shouldn't it also blur the field (as opposed to Enter)? (This also seems like something that could be a configurable prop, though if we're ok with not preserving existing behavior then it wouldn't really be necessary.)

@mattheyan mattheyan merged commit 58ddafa into cognitoforms:cognito May 4, 2024
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.

3 participants