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

fix: ComboBox show all items on open #2328

Merged
merged 7 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/components/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@react-spectrum/theme-default": "^3.5.1",
"@react-spectrum/toast": "^3.0.0-beta.16",
"@react-spectrum/utils": "^3.11.5",
"@react-types/combobox": "3.13.1",
"@react-types/radio": "^3.8.1",
"@react-types/shared": "^3.22.1",
"@react-types/textfield": "^3.9.1",
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/spectrum/comboBox/ComboBox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type { NormalizedItem } from '../utils';
import { type PickerPropsT, usePickerProps } from '../picker';

export type ComboBoxProps = PickerPropsT<SpectrumComboBoxProps<NormalizedItem>>;

export { type MenuTriggerAction } from '@react-types/combobox';
Copy link
Member

Choose a reason for hiding this comment

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

Aside - not our naming, but I was worried this name would conflict with something from the @react-types/menu package. But there they have MenuTriggerType.
I don't know why they didn't call this ComboBoxTriggerType or something instead, but whatever.

export { SpectrumComboBox };

export const ComboBox = React.forwardRef(function ComboBox(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,14 @@ describe('usePickerScrollOnOpen', () => {
expect(result.current.ref).toBe(mockUsePopoverOnScrollRefResult.ref);
});

it.each([true, false])(
'should return a callback that calls popoverOnOpenChange and onOpenChange: %s',
isOpen => {
it.each([
[true, undefined],
[false, undefined],
[true, 'input'],
[false, 'input'],
] as const)(
'should return a callback that calls popoverOnOpenChange and onOpenChange: %s, %s',
(isOpen, menuTrigger) => {
const { result } = renderHook(() =>
usePickerScrollOnOpen({
getInitialScrollPosition,
Expand All @@ -60,12 +65,12 @@ describe('usePickerScrollOnOpen', () => {
})
);

result.current.onOpenChange(isOpen);
result.current.onOpenChange(isOpen, menuTrigger);

expect(mockUsePopoverOnScrollRefResult.onOpenChange).toHaveBeenCalledWith(
isOpen
);
expect(onOpenChange).toHaveBeenCalledWith(isOpen);
expect(onOpenChange).toHaveBeenCalledWith(isOpen, menuTrigger);
}
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,17 @@ import {
findSpectrumPickerScrollArea,
usePopoverOnScrollRef,
} from '@deephaven/react-hooks';
import type { MenuTriggerAction } from '../comboBox';

export interface UsePickerScrollOnOpenOptions {
getInitialScrollPosition?: () => Promise<number | null | undefined>;
onScroll: (event: Event) => void;
onOpenChange?: (isOpen: boolean) => void;
onOpenChange?: (isOpen: boolean, menuTrigger?: MenuTriggerAction) => void;
}

export interface UsePickerScrollOnOpenResult<THtml extends HTMLElement> {
ref: DOMRef<THtml>;
onOpenChange: (isOpen: boolean) => void;
onOpenChange: (isOpen: boolean, menuTrigger?: MenuTriggerAction) => void;
}

/**
Expand All @@ -37,11 +38,11 @@ export function usePickerScrollOnOpen<THtml extends HTMLElement = HTMLElement>({
);

const onOpenChangeInternal = useCallback(
(isOpen: boolean): void => {
(isOpen: boolean, menuTrigger?: MenuTriggerAction): void => {
// Attach scroll event handling
popoverOnOpenChange(isOpen);

onOpenChange?.(isOpen);
onOpenChange?.(isOpen, menuTrigger);
},
[onOpenChange, popoverOnOpenChange]
);
Expand Down
43 changes: 41 additions & 2 deletions packages/jsapi-components/src/spectrum/ComboBox.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import {
ComboBoxNormalized,
type MenuTriggerAction,
type NormalizedItem,
type SpectrumComboBoxProps,
} from '@deephaven/components';
import { useCallback } from 'react';
import { useCallback, useRef } from 'react';
import { type PickerWithTableProps } from './PickerProps';
import { usePickerProps } from './utils';

Expand All @@ -18,19 +19,57 @@ export function ComboBox(props: ComboBoxProps): JSX.Element {
...pickerProps
} = usePickerProps<ComboBoxProps>(props);

const isOpenRef = useRef(false);
const inputValueRef = useRef('');

const onInputChange = useCallback(
(value: string) => {
onInputChangeInternal?.(value);
onSearchTextChange(value);

// Only apply search text if ComboBox is open.
if (isOpenRef.current) {
onSearchTextChange(value);
}
// When the ComboBox is closed, `onInputChange` may have been called as a
// result of user search input, ComboBox selection, or by selected key
// prop changes. We can't determine the source here, so we clear the search
// text and store the search value so that the list is unfiltered the next
// time the ComboBox is opened. We also store the search value so we can
// re-apply it in `onOpenChange` if the ComboBox is opened by user search
// input.
else {
onSearchTextChange('');
inputValueRef.current = value;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be set in all cases? Everything else looks good.

Copy link
Contributor Author

@bmingles bmingles Jan 9, 2025

Choose a reason for hiding this comment

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

I'm on the fence on this one. My reasoning for only putting it here was that the only consumer should be the menuTrigger === 'input' case in onOpenChange which should only follow this else case. I was trying to conceptually couple them in an attempt to make the code easier to reason about. That said, there's probably not any harm in setting it for all cases, and there could be an edge case that needs it that I'm unaware of. In general, reasoning about the relationship of these 2 functions is a bit of a pain.

}
},
[onInputChangeInternal, onSearchTextChange]
);

const onOpenChange = useCallback(
(isOpen: boolean, menuTrigger?: MenuTriggerAction) => {
pickerProps.onOpenChange?.(isOpen);

// Reset the search text when the ComboBox is closed.
if (!isOpen) {
onSearchTextChange('');
}
// Restore search text when ComboBox has been opened by user input.
else if (menuTrigger === 'input') {
onSearchTextChange(inputValueRef.current);
}

// Store the open state so that `onInputChange` has access to it.
isOpenRef.current = isOpen;
},
[onSearchTextChange, pickerProps]
);

return (
<ComboBoxNormalized
// eslint-disable-next-line react/jsx-props-no-spreading
{...pickerProps}
onInputChange={onInputChange}
onOpenChange={onOpenChange}
/>
);
}
Expand Down
Loading