-
Notifications
You must be signed in to change notification settings - Fork 8
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
Remove combobox autocomplete modes #2376
base: main
Are you sure you want to change the base?
Conversation
@@ -525,7 +519,7 @@ describe('Combobox', () => { | |||
// prettier-ignore | |||
const viewTemplate = html` | |||
<${comboboxTag} | |||
autocomplete="inline" | |||
autocomplete="list" |
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.
This change causes the test in this describe block to fail both with and without the other changes in this PR. I haven't yet investigated why: interactively it behaves correctly to scroll the selected item into view when autocomplete="list"
.
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.
So, the issue is that when you change to list
the items in the dropdown are limited to what has been typed in the input, whereas before they were not. Changing this to none
(or simply removing this attribute), in addition to a change needed in the focusAndScrollOptionIntoView
implementation (see this commit).
….com/ni/nimble into deprecate-combobox-autocomplete-both
this.firstSelectedOption?.scrollIntoView({ | ||
block: 'nearest' | ||
}); | ||
if (this.firstSelectedOption) { |
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.
@jattasNI and @rajsitem the code I've removed here was partially responsible for keeping a test from passing. Essentially document.activeElement
had not been updated to something contained within the Combobox
. While I can't say I confidently know why that is, the removed logic still seems wholly unnecessary. I see no reason why we have to explicitly place focus on the input
in the template when scrolling the selected option into view, nor a reason to gate this on the currently focused element already being within the Combobox
. This method should just be concerned with scrolling the current selected item into view.
Pull Request
🤨 Rationale
Fixes #2347. In that issue we decided to deprecate the combobox autocomplete modes
both
andinline
because they are too aggressive about filtering out options that should match what the user typed but don't match what the component autocompleted. Short term, clients can either useautocomplete="list"
or switch to a filterablenimble-select
. Long term we may change combobox autocomplete to behave more like select.👩💻 Implementation
In this SLE PR I'm updating places that used "both" to now use "list".
🧪 Testing
✅ Checklist