Consistent id's with svelte-search #19
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pass Typeahead's id property to Search to ensure proper `aria-labelledby` references. Updated Typeahead wrapper div id to not clash with Search's input id (this feels slightly hackish to me, but avoids separate PRs to Search). Removed an unreferenced (and oddly defined?) CSS class, updated an inner aria-labelledby reference (this is honestly a guess, as I'm not exactly an aria expert...) and ensured unique id's on the result list items.
metonym
requested changes
Mar 14, 2021
| @@ -174,15 +175,14 @@ | |||
| /> | |||
| {#if !hideDropdown && results.length > 0} | |||
| <ul | |||
| class:svelte-typeahead-list={true} | |||
There was a problem hiding this comment.
Could you restore the class name? It's a breaking change that's unrelated to the PR. I'd prefer to first patch the component before bumping a major version for this alone.
metonym
approved these changes
Mar 14, 2021
metonym
approved these changes
Mar 14, 2021
metonym
pushed a commit
to metonym/svelte-search
that referenced
this pull request
Mar 15, 2021
* Option to remove form aria attributes metonym/svelte-typeahead#19 did not solve the Lighthouse issues, but removing the `role` and `aria-labelledby` from the form does (Typeahead's `[data-svelte-typeahead]` wrapper should hopefully retain all the accessibility properties). If the PR is accepted, Typeahead will need to be updated use this non-default option. * Update type
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pass Typeahead's id property to Search to ensure proper
aria-labelledbyreferences.Updated Typeahead wrapper div id to not clash with Search's input id (this feels slightly hackish to me, but avoids separate PRs to Search).
Removed an unreferenced (and oddly defined?) CSS class, updated an inner aria-labelledby reference (this is honestly a guess, as I'm not exactly an aria expert...) and ensured unique id's on the result list items.