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 option to auto navigate to the first item on open #81

Merged
merged 6 commits into from
Feb 22, 2024
Merged

Conversation

anleac
Copy link

@anleac anleac commented Feb 22, 2024

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:

  • Do nothing on open.
  • Auto default to the first option, but not have it selected (this is what would happen currently if firstDefaultOption is true).
  • Auto navigate to the first option on open (if there is an appropriate option), this is the new behaviour.

This PR does the following:

  • Replaces the firstDefaultOption boolean with a type config of three options
  • Adds some basic tests

More than happy to update the naming if people feel strongly here.

@anleac anleac requested a review from a team as a code owner February 22, 2024 09:36
src/index.ts Outdated Show resolved Hide resolved
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')
}
Copy link
Member

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.

Copy link
Author

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:

this.indicateDefaultOption()
- where we don't want to call navigate on. Wdyt?

Copy link
Member

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')?)

Copy link
Author

Choose a reason for hiding this comment

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

Agreed! Addressed here: 1ef750e

Copy link
Member

@iansan5653 iansan5653 left a 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

src/index.ts Show resolved Hide resolved
Copy link
Member

@iansan5653 iansan5653 left a 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.

Andrew Leach and others added 3 commits February 22, 2024 17:25
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.

2 participants