-
Notifications
You must be signed in to change notification settings - Fork 20
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 option to auto navigate to the first item on open #81
Conversation
src/index.ts
Outdated
@@ -77,13 +81,19 @@ export default class Combobox { | |||
} | |||
|
|||
indicateDefaultOption(): void { | |||
if (this.defaultFirstOption) { | |||
if (this.firstOptionSelectionMode === 'selected') { | |||
Array.from(this.list.querySelectorAll<HTMLElement>('[role="option"]:not([aria-disabled="true"])')) | |||
.filter(visible)[0] | |||
?.setAttribute('data-combobox-option-default', 'true') | |||
} |
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.
Would it work to add an else if (this.firstOptionSelectionMode === 'focused') {this.navigate(1)}
case here instead of defining a separate focusDefaultOptionIfNeeded
method? That way we only have to call one function in start
and we can't forget to add the second case if we call indicateDefaultOption
elsewhere.
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 opt'ed for a new method because this method was also being called here:
Line 136 in c28985b
this.indicateDefaultOption() |
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.
ooh I see. I think I still would rather combine the methods, but then maybe here we add a conditional if (this.firstOptionSelectionMode === 'active')
(or if (this.firstOptionSelectionMode !== 'selected')
?)
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.
Agreed! Addressed here: 1ef750e
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.
Looks great! Just a documentation 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.
Oh! We should also update the docs in the README - I just realized they describe the settings as well. We could probably use the same text as in the proposed comment.
Co-authored-by: Ian Sanders <[email protected]>
This is an extension to the
firstDefaultOption
prop, and infact, a replacement, but extending an additional mode to this to now allow for the following three behaviours as a config prop:firstDefaultOption
is true).This PR does the following:
firstDefaultOption
boolean with a type config of three optionsMore than happy to update the naming if people feel strongly here.