Skip to content

refactor: move internal parts and utils from select to listbox #5204

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.

Sign up for GitHub

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

Merged
amanabiy merged 4 commits into from
Nov 6, 2026
Merged

refactor: move internal parts and utils from select to listbox #5204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ffd401
refactor: Move Select type-ahead hook to listbox
amanabiy Oct 29, 2026
e700d81
refactor: Share one option item between Select and Multiselect
amanabiy Oct 29, 2026
10b678b
refactor: Move PlainList to listbox
amanabiy Oct 29, 2026
8875ae1
test: Remove duplicate getItemProps test
amanabiy Oct 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pages/dropdown/list-with-sticky-item.page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import React, { useContext, useRef, useState } from 'react';
import Box from '~components/box';
import Button from '~components/button';
import Dropdown from '~components/dropdown';
import PlainList from '~components/internal/components/listbox/parts/plain-list';
import { DropdownOption } from '~components/internal/components/option/interfaces';
import PlainList from '~components/select/parts/plain-list';
import VirtualList from '~components/select/parts/virtual-list';
import SpaceBetween from '~components/space-between';

Expand Down Expand Up @@ -95,7 +95,7 @@ export default function MultiselectPage() {
filteringValue={''}
firstOptionSticky={true}
highlightType={{ type: 'mouse', moveFocus: false }}
useInteractiveGroups={true}
selectionType="multi"
/>
}
/>
Expand Down
11 changes: 5 additions & 6 deletions pages/select/item.permutations.page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@
// SPDX-License-Identifier: Apache-2.0
import React from 'react';

import Item, { ItemProps } from '~components/internal/components/listbox/parts/item';
import { DropdownOption } from '~components/internal/components/option/interfaces';
import Item from '~components/select/parts/item';
import { ItemProps } from '~components/select/parts/item';

import createPermutations from '../utils/permutations';
import PermutationsView from '../utils/permutations-view';
Expand Down Expand Up @@ -100,7 +99,7 @@ const permutations = createPermutations<Omit<ItemProps, 'index'>>([
highlighted: [false, true],
selected: [false, true],
highlightType: ['keyboard'],
hasCheckbox: [true],
selectionType: ['multi'],
},
{
option: [options.complexOption],
Expand All @@ -126,21 +125,21 @@ const permutations = createPermutations<Omit<ItemProps, 'index'>>([
},
{
option: [options.complexDisabledOptionWithCustomSvg],
hasCheckbox: [true],
selectionType: ['multi'],
},
{
option: [options.simpleOption, options.complexOption],
highlighted: [false, true],
highlightType: ['keyboard'],
selected: [true],
hasCheckbox: [false],
selectionType: ['single'],
},
{
option: [options.complexOptionGroup],
highlighted: [false],
highlightType: ['keyboard'],
selected: [true],
hasCheckbox: [false, true],
selectionType: ['single', 'multi'],
},
]);

Expand Down
27 changes: 0 additions & 27 deletions src/internal/components/listbox/__tests__/get-item-props.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ const defaultItemProps = {
index: 1,
getOptionProps: getOptionProps(false, false),
filteringValue: 'child',
checkboxes: false,
};

describe('getItemProps', () => {
Expand All @@ -68,29 +67,6 @@ describe('getItemProps', () => {
'data-test-index': 1,
'data-mouse-target': 1,
filteringValue: 'child',
hasCheckbox: false,
highlighted: false,
key: 1,
option: {
option: { label: 'Child 1' },
type: 'child',
},
selected: false,
});
});

test('should return props for the first child in the first group with checkboxes and keyboard', () => {

@cansuaa cansuaa Oct 30, 2026

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.

Why did you remove this test?

@amanabiy amanabiy Nov 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This test became a duplicate. Its only difference from the first test was the checkboxes option. After merging the Select and Multiselect rows into one, whether a row shows a checkbox depends on whether it's a Select or Multiselect row, so getItemProps no longer takes that option. Without it, the two tests were identical, so I kept one. Here is the first test test

const props = getItemProps({
...defaultItemProps,
checkboxes: true,
});
expect(props).toEqual({
'data-child-index': 1,
'data-group-index': 1,
'data-test-index': 1,
'data-mouse-target': 1,
filteringValue: 'child',
hasCheckbox: true,
highlighted: false,
key: 1,
option: {
Expand All @@ -112,7 +88,6 @@ describe('getItemProps', () => {
'data-test-index': 1,
'data-mouse-target': 1,
filteringValue: 'child',
hasCheckbox: false,
highlighted: true,
key: 1,
option: {
Expand All @@ -136,7 +111,6 @@ describe('getItemProps', () => {
'data-test-index': 1,
'data-mouse-target': 1,
filteringValue: 'child',
hasCheckbox: false,
highlighted: false,
key: 1,
option: {
Expand All @@ -159,7 +133,6 @@ describe('getItemProps', () => {
'data-mouse-target': 3,
'data-test-index': 3,
filteringValue: 'child',
hasCheckbox: false,
highlighted: false,
key: 3,
option: {
Expand Down
12 changes: 6 additions & 6 deletions ...elect/__tests__/use-native-search.test.ts → .../listbox/__tests__/use-type-ahead.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

import { act, renderHook } from '../../__tests__/render-hook';
import type { DropdownOption } from '../../internal/components/option/interfaces';
import { useNativeSearch } from '../utils/use-native-search';
import { act, renderHook } from '../../../../__tests__/render-hook';
import type { DropdownOption } from '../../option/interfaces';
import { useTypeAhead } from '../use-type-ahead';

const options: DropdownOption[] = [
{ option: { label: 'option 1', value: 'option1' } },
Expand All @@ -19,8 +19,8 @@ const initialProps = {
highlightedOption: null,
};

describe('useNativeSearch', () => {
const hook = renderHook(useNativeSearch, {
describe('useTypeAhead', () => {

@cansuaa cansuaa Oct 30, 2026

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.

What is "type ahead"?

@amanabiy amanabiy Nov 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's the same feature that used to be called useNativeSearch: when the list has focus, typing letters jumps to the first option that starts with them, like in a native . I renamed it because "type-ahead" is what the ARIA listbox pattern calls it: (https://www.w3.org/WAI/ARIA/apg/patterns/listbox/#keyboardinteraction).

const hook = renderHook(useTypeAhead, {
initialProps: initialProps,
});

Expand All @@ -47,7 +47,7 @@ describe('useNativeSearch', () => {
});

test('should not highlight an option when isEnabled is false', () => {
const hook = renderHook(useNativeSearch, {
const hook = renderHook(useTypeAhead, {
initialProps: {
...initialProps,
isEnabled: false,
Expand Down
4 changes: 1 addition & 3 deletions src/internal/components/listbox/parts/get-item-props.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,14 @@ interface ItemProps {
index: number;
getOptionProps: any;
filteringValue: string;
checkboxes: boolean;
}

export const getItemProps = ({ option, index, getOptionProps, filteringValue, checkboxes = false }: ItemProps) => {
export const getItemProps = ({ option, index, getOptionProps, filteringValue }: ItemProps) => {
const optionProps = getOptionProps(option, index);
optionProps.filteringValue = filteringValue;
const { inGroupIndex, groupIndex, throughIndex } = getTestOptionIndexes(option) || {};
return {
...optionProps,
hasCheckbox: checkboxes,

@amanabiy amanabiy Oct 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hasCheckbox is not needed anymore because on last PR I introduced selectionType: 'single' | 'mult'. The multi row always draws the checkbox and the single row never does.

['data-group-index']: groupIndex,
['data-child-index']: inGroupIndex,
['data-test-index']: throughIndex,
Expand Down
Loading
Loading