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

lib: Some TypeaheadSelect and MultiTypeaheadSelect improvements #21482

Merged
merged 2 commits into from
Jan 5, 2025

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Jan 3, 2025

No description provided.

@mvollmer
Copy link
Member Author

mvollmer commented Jan 3, 2025

For c-podman.

In the TypeaheadSelect component, the focusedItemIndex is for the
filteredSelections array, not for the selectOptions one. Thus without
this fix, we would get the aria stuff wrong. We might even end up
accessing non-existing items, when focusedItemIndex refers to a "No
results found" fake entry in filteredSelections with index 0 while
selectOptions is empty.

The fake entries are now properly disabled and will be skipped by key
navigation in both TypeaheadSelect and MultiTypeaheadSelect.

Also, key navigation relies on there being at least one option that
will not be skipped. Thus, we need to us isEnabledMenu instead of
isMenu when deciding whether to return early. This got broken in
8d9f97b.
@mvollmer mvollmer force-pushed the lib-typeahead-improvs branch from de155fc to 09d94a2 Compare January 3, 2025 12:08
@mvollmer mvollmer requested a review from tomasmatus January 3, 2025 12:09
@@ -535,6 +543,7 @@ export const TypeaheadSelectBase: React.FunctionComponent<TypeaheadSelectProps>
);
})}
</SelectList>
{ footer && <MenuFooter>{footer}</MenuFooter> }
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

@martinpitt martinpitt merged commit 225e9c2 into cockpit-project:main Jan 5, 2025
85 checks passed
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.

3 participants