Skip to content

Add option to auto navigate to the first item on open#81

Merged
anleac merged 6 commits into
mainfrom
add-new-option
Feb 22, 2024
Merged

Add option to auto navigate to the first item on open#81
anleac merged 6 commits into
mainfrom
add-new-option

Conversation

@anleac

@anleac anleac commented Feb 22, 2024

Copy link
Copy Markdown

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
Comment thread src/index.ts Outdated
Comment thread src/index.ts Outdated
Array.from(this.list.querySelectorAll<HTMLElement>('[role="option"]:not([aria-disabled="true"])'))
.filter(visible)[0]
?.setAttribute('data-combobox-option-default', 'true')
}

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@iansan5653 iansan5653 left a comment

Copy link
Copy Markdown
Member

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

Comment thread src/index.ts

@iansan5653 iansan5653 left a comment

Copy link
Copy Markdown
Member

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
Co-authored-by: Ian Sanders <iansan5653@github.com>
@anleac anleac merged commit 6a87759 into main Feb 22, 2024
@anleac anleac deleted the add-new-option branch February 22, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants