Skip to content

ref(scraps): make value and onChange required on compactSelect #103654

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
TkDodo merged 8 commits into from
Nov 19, 2025
Merged

ref(scraps): make value and onChange required on compactSelect #103654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8e0a2ab
ref(scraps): make value and onChange required on compactSelect
TkDodo Nov 19, 2025
c98f84e
ref: remove experiment
TkDodo Nov 19, 2025
6b38111
ref: fallback isn't necessary
TkDodo Nov 19, 2025
64e32ba
ref: move onChange back to where it was
TkDodo Nov 19, 2025
d80edcf
ref: bring back disabled: false
TkDodo Nov 19, 2025
3ae5fa1
ref: remove low quality jsdocs
TkDodo Nov 19, 2025
83e4487
Merge branch 'master' into tkdodo/ref/compact-select-required-value-a…
TkDodo Nov 19, 2025
ab2afaf
fix: jest mock fns
TkDodo Nov 19, 2025
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
10 changes: 3 additions & 7 deletions static/app/components/charts/optionSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,18 @@ type BaseProps = {
interface SingleProps
extends Omit<
SingleSelectProps<string>,
'onChange' | 'defaultValue' | 'multiple' | 'title'
'onChange' | 'defaultValue' | 'multiple' | 'title' | 'value'
>,
BaseProps {
onChange: (value: string) => void;
selected: string;
defaultValue?: string;
multiple?: false;
}

interface MultipleProps
extends Omit<
MultipleSelectProps<string>,
'onChange' | 'defaultValue' | 'multiple' | 'title'
'onChange' | 'defaultValue' | 'multiple' | 'title' | 'value'
>,
BaseProps {
multiple: true;
Expand All @@ -48,7 +47,6 @@ function OptionSelector({
title,
featureType,
multiple,
defaultValue,
closeOnSelect,
...rest
}: SingleProps | MultipleProps) {
Expand All @@ -68,7 +66,6 @@ function OptionSelector({
return {
multiple,
value: selected,
defaultValue,
onChange: (sel: Array<SelectOption<string>>) => {
onChange?.(sel.map(o => o.value));
},
Expand All @@ -79,11 +76,10 @@ function OptionSelector({
return {
multiple,
value: selected,
defaultValue,
onChange: (opt: any) => onChange?.(opt.value),
closeOnSelect,
};
}, [multiple, selected, defaultValue, onChange, closeOnSelect]);
}, [multiple, selected, onChange, closeOnSelect]);

function isOptionDisabled(option: SelectOptionWithKey<string>) {
return Boolean(
Expand Down
36 changes: 36 additions & 0 deletions static/app/components/core/compactSelect/compactSelect.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ describe('CompactSelect', () => {
it('renders', async () => {
render(
<CompactSelect
value={undefined}
onChange={jest.fn()}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand All @@ -21,6 +23,8 @@ describe('CompactSelect', () => {
render(
<CompactSelect
disabled
value={undefined}
onChange={jest.fn()}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand All @@ -34,6 +38,8 @@ describe('CompactSelect', () => {
render(
<CompactSelect
menuTitle="Menu title"
value={undefined}
onChange={jest.fn()}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand All @@ -53,6 +59,7 @@ describe('CompactSelect', () => {
<CompactSelect
value="opt_one"
menuTitle="Menu A"
onChange={jest.fn()}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand All @@ -61,6 +68,7 @@ describe('CompactSelect', () => {
<CompactSelect
value="opt_three"
menuTitle="Menu B"
onChange={jest.fn()}
options={[
{value: 'opt_three', label: 'Option Three'},
{value: 'opt_four', label: 'Option Four'},
Expand Down Expand Up @@ -110,6 +118,7 @@ describe('CompactSelect', () => {
const mock = jest.fn();
render(
<CompactSelect
value={undefined}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand Down Expand Up @@ -138,6 +147,7 @@ describe('CompactSelect', () => {
{value: 'opt_two', label: 'Option Two'},
]}
onChange={mock}
value={undefined}
/>
);

Expand Down Expand Up @@ -165,6 +175,7 @@ describe('CompactSelect', () => {
{value: '"opt_two"', label: 'Option Two'},
]}
onChange={mock}
value={undefined}
/>
);

Expand All @@ -186,6 +197,7 @@ describe('CompactSelect', () => {
<CompactSelect
triggerProps={{prefix: 'Prefix'}}
value="opt_one"
onChange={jest.fn()}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand All @@ -206,6 +218,8 @@ describe('CompactSelect', () => {
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
]}
value={undefined}
onChange={jest.fn()}
/>
);

Expand All @@ -224,6 +238,8 @@ describe('CompactSelect', () => {
it('can search with sections', async () => {
render(
<CompactSelect
value={undefined}
onChange={jest.fn()}
searchable
searchPlaceholder="Search here…"
options={[
Expand Down Expand Up @@ -265,6 +281,8 @@ describe('CompactSelect', () => {
it('can limit the number of options', async () => {
render(
<CompactSelect
value={undefined}
onChange={jest.fn()}
sizeLimit={2}
sizeLimitMessage="Use search for more options…"
searchable
Expand Down Expand Up @@ -316,6 +334,8 @@ describe('CompactSelect', () => {
<CompactSelect
multiple
onSectionToggle={mock}
value={undefined}
onChange={jest.fn()}
options={[
{
key: 'section-1',
Expand Down Expand Up @@ -424,6 +444,8 @@ describe('CompactSelect', () => {
render(
<CompactSelect
onClose={onCloseMock}
value={undefined}
onChange={jest.fn()}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand All @@ -449,6 +471,7 @@ describe('CompactSelect', () => {
render(
<CompactSelect
grid
value={undefined}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand All @@ -473,6 +496,7 @@ describe('CompactSelect', () => {
<CompactSelect
grid
multiple
value={undefined}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
Expand Down Expand Up @@ -506,6 +530,7 @@ describe('CompactSelect', () => {
{value: '"opt_two"', label: 'Option Two'},
]}
onChange={mock}
value={undefined}
/>
);

Expand All @@ -532,6 +557,7 @@ describe('CompactSelect', () => {
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
]}
onChange={jest.fn()}
/>
);
expect(
Expand All @@ -549,6 +575,8 @@ describe('CompactSelect', () => {
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
]}
value={undefined}
onChange={jest.fn()}
/>
);

Expand Down Expand Up @@ -576,6 +604,8 @@ describe('CompactSelect', () => {
{value: 'opt_two', label: 'Option Two'},
{value: 'opt_three', label: 'Option Three'},
]}
value={undefined}
onChange={jest.fn()}
/>
);

Expand Down Expand Up @@ -614,6 +644,8 @@ describe('CompactSelect', () => {
grid
multiple
onSectionToggle={mock}
value={undefined}
onChange={jest.fn()}
options={[
{
key: 'section-1',
Expand Down Expand Up @@ -722,6 +754,8 @@ describe('CompactSelect', () => {
render(
<CompactSelect
grid
value={undefined}
onChange={jest.fn()}
onClose={onCloseMock}
options={[
{value: 'opt_one', label: 'Option One'},
Expand All @@ -748,6 +782,8 @@ describe('CompactSelect', () => {
render(
<CompactSelect
grid
value={undefined}
onChange={jest.fn()}
options={[
{
value: 'opt_one',
Expand Down
20 changes: 14 additions & 6 deletions static/app/components/core/compactSelect/composite.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ describe('CompactSelect', () => {
<CompositeSelect disabled>
<CompositeSelect.Region
label="Region 1"
onChange={() => {}}
onChange={jest.fn()}
value={undefined}
options={[
{value: 'choice_one', label: 'Choice One'},
{value: 'choice_two', label: 'Choice Two'},
Expand All @@ -87,7 +88,8 @@ describe('CompactSelect', () => {
<CompositeSelect>
<CompositeSelect.Region
label="Region 1"
onChange={() => {}}
onChange={jest.fn()}
value={undefined}
options={[
{value: 'choice_one', label: 'Choice One'},
{value: 'choice_two', label: 'Choice Two'},
Expand All @@ -96,7 +98,8 @@ describe('CompactSelect', () => {
<CompositeSelect.Region
multiple
label="Region 2"
onChange={() => {}}
onChange={jest.fn()}
value={undefined}
options={[
{value: 'choice_three', label: 'Choice Three'},
{value: 'choice_four', label: 'Choice Four'},
Expand Down Expand Up @@ -143,6 +146,7 @@ describe('CompactSelect', () => {
<CompositeSelect.Region
label="Region 1"
onChange={region1Mock}
value={undefined}
options={[
{value: 'choice_one', label: 'Choice One'},
{value: 'choice_two', label: 'Choice Two'},
Expand All @@ -152,6 +156,7 @@ describe('CompactSelect', () => {
multiple
label="Region 2"
onChange={region2Mock}
value={undefined}
options={[
{value: 'choice_three', label: 'Choice Three'},
{value: 'choice_four', label: 'Choice Four'},
Expand Down Expand Up @@ -212,7 +217,8 @@ describe('CompactSelect', () => {
<CompositeSelect searchable searchPlaceholder="Search placeholder…">
<CompositeSelect.Region
label="Region 1"
onChange={() => {}}
onChange={jest.fn()}
value={undefined}
options={[
{value: 'choice_one', label: 'Choice One'},
{value: 'choice_two', label: 'Choice Two'},
Expand All @@ -221,7 +227,8 @@ describe('CompactSelect', () => {
<CompositeSelect.Region
multiple
label="Region 2"
onChange={() => {}}
onChange={jest.fn()}
value={undefined}
options={[
{value: 'choice_three', label: 'Choice Three'},
{value: 'choice_four', label: 'Choice Four'},
Expand Down Expand Up @@ -262,7 +269,8 @@ describe('CompactSelect', () => {
<CompositeSelect.Region
multiple
label="Region 2"
onChange={() => {}}
onChange={jest.fn()}
value={undefined}
options={[
{value: 'choice_three', label: 'Choice Three'},
{value: 'choice_four', label: 'Choice Four'},
Expand Down
8 changes: 4 additions & 4 deletions static/app/components/core/compactSelect/list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,25 +100,25 @@ interface BaseListProps<Value extends SelectKey>
}

export interface SingleListProps<Value extends SelectKey> extends BaseListProps<Value> {
onChange: (selectedOption: SelectOption<Value>) => void;
value: Value | undefined;
/**
* Whether to close the menu. Accepts either a boolean value or a callback function
* that receives the newly selected option and returns whether to close the menu.
*/
closeOnSelect?: boolean | ((selectedOption: SelectOption<Value>) => boolean);
multiple?: false;
onChange?: (selectedOption: SelectOption<Value>) => void;
value?: Value;
}

export interface MultipleListProps<Value extends SelectKey> extends BaseListProps<Value> {
multiple: true;
onChange: (selectedOptions: Array<SelectOption<Value>>) => void;
value: Value[] | undefined;
/**
* Whether to close the menu. Accepts either a boolean value or a callback function
* that receives the newly selected options and returns whether to close the menu.
*/
closeOnSelect?: boolean | ((selectedOptions: Array<SelectOption<Value>>) => boolean);
onChange?: (selectedOptions: Array<SelectOption<Value>>) => void;
value?: Value[];
}

/**
Expand Down
8 changes: 8 additions & 0 deletions static/app/components/core/compactSelect/listBox/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ describe('ListBox', () => {
it('hides details overlay when mouse leaves the list', async () => {
render(
<CompactSelect
onChange={jest.fn()}
value={undefined}
options={[
{
value: 'opt_one',
Expand Down Expand Up @@ -42,6 +44,8 @@ describe('ListBox', () => {
it('shows different details when hovering over different options', async () => {
render(
<CompactSelect
onChange={jest.fn()}
value={undefined}
options={[
{
value: 'opt_one',
Expand Down Expand Up @@ -81,6 +85,8 @@ describe('ListBox', () => {
it('does not show details overlay when showDetailsInOverlay is false', async () => {
render(
<CompactSelect
onChange={jest.fn()}
value={undefined}
options={[
{
value: 'opt_one',
Expand All @@ -105,6 +111,8 @@ describe('ListBox', () => {
it('maintains keyboard navigation focus when hovering', async () => {
render(
<CompactSelect
onChange={jest.fn()}
value={undefined}
options={[
{
value: 'opt_one',
Expand Down
Loading
Loading