Skip to content

fix: bound signal updates on the combobox #968

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

Merged
merged 32 commits into from
Sep 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
b7bf086
initial tests
thejackshelton Sep 20, 2024
56613a9
get values with arrays instead
thejackshelton Sep 21, 2024
f3f9d0e
reactively update
thejackshelton Sep 21, 2024
6b22b90
make selection manager cleaner
thejackshelton Sep 21, 2024
160886d
remove unnecessary addition
thejackshelton Sep 21, 2024
d244a18
fix: onChange$ only executes on client & simpler disabled state
thejackshelton Sep 21, 2024
e321b55
feat: simpler invalid check
thejackshelton Sep 21, 2024
d1be585
remove early return
thejackshelton Sep 21, 2024
5bffa49
feat: simplify inline component
thejackshelton Sep 21, 2024
ed7b86f
simpler initial value naming
thejackshelton Sep 21, 2024
83258f1
refactor: better context names
thejackshelton Sep 21, 2024
4c67462
fix: combobox closes on enter key
thejackshelton Sep 22, 2024
7a77229
fix: combobox properly resets when empty
thejackshelton Sep 22, 2024
4ba2d9d
get correct initial value reactively
thejackshelton Sep 22, 2024
6a8efd0
fix: highlight jumping
thejackshelton Sep 22, 2024
ccc7899
multiple shows correct display
thejackshelton Sep 22, 2024
a2c67d7
fix: undefined does not become part of the input
thejackshelton Sep 22, 2024
ae99c8b
fix: handle proper empty input
thejackshelton Sep 22, 2024
c9969d1
feat: use local index instead
thejackshelton Sep 22, 2024
f55d297
fix: last filtered item now gets highlighted on down key
thejackshelton Sep 22, 2024
d8e8c21
fix: preserve item disabled state
thejackshelton Sep 22, 2024
a88b6b5
fix: combobox empty now properly shows for both filter and non-filter…
thejackshelton Sep 22, 2024
01503a0
fix: removing items on backspace
thejackshelton Sep 22, 2024
908b0b2
respect input's given signal first
thejackshelton Sep 22, 2024
bc397f9
refactor: remove all the logs
thejackshelton Sep 22, 2024
6605f9e
fix: combobox scrolling
thejackshelton Sep 22, 2024
18eefb9
fix: form submissions
thejackshelton Sep 22, 2024
40a3d9b
fix: pw tests
thejackshelton Sep 22, 2024
7d30b0c
refactor: remove test since behavior is no longer warranted
thejackshelton Sep 22, 2024
0eab200
fix: mouse should not affect initial keyboard highlight
thejackshelton Sep 22, 2024
31f4a0a
feat: changeset
thejackshelton Sep 22, 2024
aabfc7a
merge w/ main
thejackshelton Sep 22, 2024
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
31 changes: 31 additions & 0 deletions .changeset/funny-pens-vanish.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
---
'@qwik-ui/headless': patch
---

# Combobox Improvements

## 🔄 Reactive Improvements

- Better handling of array-based values
- Improved handling of initial and reactive values

## 🐛 Key Bug Fixes

- Fixed highlight jumping issues
- Enhanced empty input handling
- Better filtered item highlighting

## 🖱️ Interaction Enhancements

- Smoother scrolling experience
- Improved keyboard and mouse coordination

## 🚀 Performance Optimizations

- More efficient item filtering

## 🧪 Reliability

- Added tests for reactivity handling, item unselection, and mouse-to-pointer interaction switching

- Improved handling of edge cases in user interactions
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,17 @@ export type ComboboxContext = {
labelRef: Signal<HTMLDivElement | undefined>;
controlRef: Signal<HTMLDivElement | undefined>;
selectedValueSetSig: Signal<Set<string>>;
selectedValuesSig: Signal<string | string[]>;
filteredIndexSetSig: Signal<Set<number>>;
highlightedIndexSig: Signal<number | null>;
currDisplayValueSig: Signal<string | string[] | undefined>;
displayValuesSig: Signal<string | string[] | undefined>;
isMouseOverPopupSig: Signal<boolean>;
isKeyboardFocusSig: Signal<boolean>;
disabledIndexSetSig: Signal<Set<number>>;
removeOnBackspace: boolean;
hasVisibleItemsSig: Signal<boolean>;
isNoItemsSig: Signal<boolean>;
initialLoadSig: Signal<boolean>;
_value: string | undefined;
initialValue: string | undefined;

loop: boolean;
multiple: boolean | undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export const HComboboxEmpty = component$((props: PropsOf<'div'>) => {
return (
<div
data-qui-combobox-empty
data-empty={context.hasVisibleItemsSig.value ? undefined : ''}
data-empty={context.isNoItemsSig.value ? '' : undefined}
{...props}
>
<Slot />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const ComboboxHiddenSelectOption = component$(
const context = useContext(comboboxContextId);

useTask$(async function modularFormsValidation({ track }) {
track(() => context.selectedValueSetSig.value);
track(() => context.selectedValuesSig.value);

if (isServer || !nativeSelectRef.value || !optionRef.value) return;

Expand All @@ -37,7 +37,11 @@ export const ComboboxHiddenSelectOption = component$(
'Qwik UI: value not found when trying to select or unselect an item.',
);
}
optionRef.value.selected = context.selectedValueSetSig.value.has(value);

const selectedValues = context.selectedValuesSig.value;
optionRef.value.selected = Array.isArray(selectedValues)
? selectedValues.includes(value)
: selectedValues === value;
});

return (
Expand Down
165 changes: 66 additions & 99 deletions packages/kit-headless/src/components/combobox/combobox-inline.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { type JSXNode, Component } from '@builder.io/qwik';
import { Component, JSXChildren } from '@builder.io/qwik';
import { HComboboxRootImpl, HComboboxRootImplProps } from './combobox-root';
import { HComboboxItem as InternalComboboxItem } from './combobox-item';
import { HComboboxItemLabel as InternalComboboxItemLabel } from './combobox-item-label';
import { HComboboxEmpty as InternalComboboxEmpty } from './combobox-empty';
import { HComboboxErrorMessage } from './combobox-error-message';
import { findComponent, processChildren } from '../../utils/inline-component';

export type TItemsMap = Map<
number,
Expand All @@ -13,8 +14,8 @@ export type TItemsMap = Map<
export type InternalComboboxProps = {
/** When a value is passed, we check if it's an actual item value, and get its index at pre-render time.
**/
_valuePropIndex?: number | null;
_value?: string;
initialIndex?: number | null;
initialValue?: string;

/** Checks if the consumer added the label in their JSX */
_label?: boolean;
Expand All @@ -35,7 +36,7 @@ export const HComboboxRoot: Component<InternalComboboxProps & HComboboxRootImplP
props: InternalComboboxProps & HComboboxRootImplProps,
) => {
const {
children: myChildren,
children,
comboboxItemComponent: UserItem,
comboboxItemLabelComponent: UserItemLabel,
...rest
Expand All @@ -47,121 +48,87 @@ export const HComboboxRoot: Component<InternalComboboxProps & HComboboxRootImplP

// source of truth
const itemsMap = new Map();

let currItemIndex = 0;
let initialIndex = null;
let initialValue;

let isItemDisabled = false;
let givenItemValue = null;
let valuePropIndex = null;
let _value;

// user adds value prop on Item component
let givenItemValue: string | null = null;

let hasEmptyComp = false;
let hasErrorComp = false;

const childrenToProcess = (
Array.isArray(myChildren) ? [...myChildren] : [myChildren]
) as Array<JSXNode>;
findComponent(HComboboxItem, (itemProps) => {
itemProps._index = currItemIndex;

isItemDisabled = itemProps.disabled === true;

if (itemProps.value) {
givenItemValue = itemProps.value as string;
}

// the default case isn't handled here, so we need to process the children to get to the label component
if (itemProps.children) {
return processChildren(itemProps.children as JSXChildren);
}
});

findComponent(HComboboxItemLabel, (labelProps) => {
const displayValue = labelProps.children as string;

while (childrenToProcess.length) {
const child = childrenToProcess.shift();
// distinct value, or the display value is the same as the value
const value = (givenItemValue !== null ? givenItemValue : displayValue) as string;

if (!child) {
continue;
itemsMap.set(currItemIndex, { value, displayValue, disabled: isItemDisabled });

if (props.value && props.multiple) {
throw new Error(
`Qwik UI: When in multiple selection mode, the value prop is disabled. Use the bind:value prop's initial signal value instead.`,
);
}

if (Array.isArray(child)) {
childrenToProcess.unshift(...child);
continue;
// if the current option value is equal to the initial value
if (value === props.value) {
// minus one because it is incremented already in SelectOption
initialIndex = currItemIndex;
initialValue = value;
}

switch (child.type) {
case HComboboxItem: {
// get the index of the current option
child.props._index = currItemIndex;

isItemDisabled = child.props.disabled === true;

if (child.props.value) {
givenItemValue = child.props.value;
}

// the default case isn't handled here, so we need to process the children to get to the label component
if (child.props.children) {
const childChildren = Array.isArray(child.props.children)
? [...child.props.children]
: [child.props.children];
childrenToProcess.unshift(...childChildren);
}

break;
}

case HComboboxItemLabel: {
const displayValue = child.props.children as string;

// distinct value, or the display value is the same as the value
const value = (givenItemValue !== null ? givenItemValue : displayValue) as string;

itemsMap.set(currItemIndex, { value, displayValue, disabled: isItemDisabled });

if (props.value && props.multiple) {
throw new Error(
`Qwik UI: When in multiple selection mode, the value prop is disabled. Use the bind:value prop's initial signal value instead.`,
);
}

// if the current option value is equal to the initial value
if (value === props.value) {
// minus one because it is incremented already in SelectOption
valuePropIndex = currItemIndex;
_value = value;
}

const isString = typeof child.props.children === 'string';

if (!isString) {
throw new Error(
`Qwik UI: select item label passed was not a string. It was a ${typeof child
.props.children}.`,
);
}

// increment after processing children
currItemIndex++;

break;
}

case HComboboxEmpty: {
hasEmptyComp = true;
break;
}

case HComboboxErrorMessage: {
hasErrorComp = true;
break;
}

default: {
if (child) {
const anyChildren = Array.isArray(child.children)
? [...child.children]
: [child.children];
childrenToProcess.unshift(...(anyChildren as JSXNode[]));
}

break;
}
const isString = typeof labelProps.children === 'string';

if (!isString) {
throw new Error(
`Qwik UI: select item label passed was not a string. It was a ${typeof labelProps.children}.`,
);
}
}

// increment after processing children
currItemIndex++;
});

findComponent(HComboboxEmpty, () => {
hasEmptyComp = true;
});

findComponent(HComboboxErrorMessage, () => {
hasErrorComp = true;
});

processChildren(children);

return (
<HComboboxRootImpl
{...rest}
_valuePropIndex={valuePropIndex}
_value={_value}
initialIndex={initialIndex}
initialValue={initialValue}
_itemsMap={itemsMap}
hasEmptyComp={hasEmptyComp}
hasErrorComp={hasErrorComp}
>
{props.children}
{children}
</HComboboxRootImpl>
);
};
Loading
Loading