fix: various bug fixes (#456)

* fix(number-field): only format when enabled * fix(number-field): don't trigger `onRawValueChange` on mount when NaN * fix(select): correct type definition & empty value for multiselect * fix(text-field): clear input when controlled value set to undefined * fix(combobox): correct type definition & empty value for multiselect * fix(skeleton): correct data-animate & data-visible attribute value * fix(combobox): close list on outside click * fix(navigation-menu): incorrect animation after closed
kobaltedev · Aug 26, 2024 · 24c5ef8 · 24c5ef8
1 parent 1872bcb
commit 24c5ef8
Show file tree

Hide file tree

Showing 16 changed files with 58 additions and 29 deletions.
diff --git a/apps/docs/app.config.ts b/apps/docs/app.config.ts
@@ -113,8 +113,8 @@ export default defineConfig({
 		},
 		prerender: {
 			routes: ["/docs/core/overview/introduction"],
-			crawlLinks: true
-		}
+			crawlLinks: true,
+		},
 	},
 
 	extensions: ["mdx", "md"],

diff --git a/biome.json b/biome.json
@@ -1,7 +1,7 @@
 {
 	"$schema": "./node_modules/@biomejs/biome/configuration_schema.json",
 	"files": {
-		"ignore": ["./tsconfig.json", "*/netlify/*"]
+		"ignore": ["./tsconfig.json", "*/netlify/*", "**/package.json"]
 	},
 	"vcs": {
 		"enabled": true,

diff --git a/packages/core/src/combobox/combobox-base.tsx b/packages/core/src/combobox/combobox-base.tsx
@@ -596,9 +596,8 @@ export function ComboboxBase<
 		focusStrategy: FocusStrategy | boolean,
 		triggerMode?: ComboboxTriggerMode,
 	) => {
-
 		// If set to only open manually, ignore other triggers
-		if (local.triggerMode === 'manual' && triggerMode !== 'manual') {
+		if (local.triggerMode === "manual" && triggerMode !== "manual") {
 			return;
 		}
 

diff --git a/packages/core/src/combobox/combobox-content.tsx b/packages/core/src/combobox/combobox-content.tsx
@@ -82,11 +82,14 @@ export function ComboboxContent<T extends ValidComponent = "div">(
 		"onFocusOutside",
 	]);
 
-	const close = () => {
+	const dismiss = () => {
 		context.resetInputValue(
 			context.listState().selectionManager().selectedKeys(),
 		);
 		context.close();
+		setTimeout(() => {
+			context.close();
+		});
 	};
 
 	const onFocusOutside = (e: FocusOutsideEvent) => {
@@ -165,7 +168,7 @@ export function ComboboxContent<T extends ValidComponent = "div">(
 						local.style,
 					)}
 					onFocusOutside={onFocusOutside}
-					onDismiss={close}
+					onDismiss={dismiss}
 					{...context.dataset()}
 					{...others}
 				/>

diff --git a/packages/core/src/combobox/combobox-input.tsx b/packages/core/src/combobox/combobox-input.tsx
@@ -37,6 +37,7 @@ export interface ComboboxInputCommonProps<
 	ref: T | ((el: T) => void);
 	onInput: JSX.EventHandlerUnion<T, InputEvent>;
 	onKeyDown: JSX.EventHandlerUnion<T, KeyboardEvent>;
+	onClick: JSX.EventHandlerUnion<T, MouseEvent>;
 	onFocus: JSX.EventHandlerUnion<T, FocusEvent>;
 	onBlur: JSX.EventHandlerUnion<T, FocusEvent>;
 	onTouchEnd: JSX.EventHandlerUnion<T, TouchEvent>;
@@ -93,6 +94,7 @@ export function ComboboxInput<T extends ValidComponent = "input">(
 		[
 			"ref",
 			"disabled",
+			"onClick",
 			"onInput",
 			"onKeyDown",
 			"onFocus",
@@ -113,6 +115,14 @@ export function ComboboxInput<T extends ValidComponent = "input">(
 
 	const { fieldProps } = createFormControlField(formControlFieldProps);
 
+	const onClick: JSX.EventHandlerUnion<HTMLInputElement, MouseEvent> = (e) => {
+		callHandler(e, local.onClick);
+
+		if (context.triggerMode() === "focus" && !context.isOpen()) {
+			context.open(false, "focus");
+		}
+	};
+
 	const onInput: JSX.EventHandlerUnion<HTMLInputElement, InputEvent> = (e) => {
 		callHandler(e, local.onInput);
 
@@ -315,6 +325,7 @@ export function ComboboxInput<T extends ValidComponent = "input">(
 			aria-required={formControlContext.isRequired() || undefined}
 			aria-disabled={formControlContext.isDisabled() || undefined}
 			aria-readonly={formControlContext.isReadOnly() || undefined}
+			onClick={onClick}
 			onInput={onInput}
 			onKeyDown={onKeyDown}
 			onFocus={onFocus}

diff --git a/packages/core/src/combobox/combobox-root.tsx b/packages/core/src/combobox/combobox-root.tsx
@@ -15,7 +15,7 @@ import {
 
 export interface ComboboxSingleSelectionOptions<T> {
 	/** The controlled value of the combobox. */
-	value?: T;
+	value?: T | null;
 
 	/**
 	 * The value of the combobox when initially rendered.
@@ -24,7 +24,7 @@ export interface ComboboxSingleSelectionOptions<T> {
 	defaultValue?: T;
 
 	/** Event handler called when the value changes. */
-	onChange?: (value: T) => void;
+	onChange?: (value: T | null) => void;
 
 	/** Whether the combobox allow multiple selection. */
 	multiple?: false;
@@ -100,10 +100,10 @@ export function ComboboxRoot<
 
 	const onChange = (value: Option[]) => {
 		if (local.multiple) {
-			local.onChange?.(value as any);
+			local.onChange?.((value ?? []) as any);
 		} else {
 			// use `null` as "no value" because `undefined` mean the component is "uncontrolled".
-			local.onChange?.((value[0] ?? null) as any);
+			local.onChange?.((value[0] ?? null) as any); // TODO: maybe return undefined? breaking change!
 		}
 	};
 

diff --git a/packages/core/src/menu/menu-content-base.tsx b/packages/core/src/menu/menu-content-base.tsx
@@ -290,6 +290,8 @@ export function MenuContentBase<T extends ValidComponent = "div">(
 
 	createEffect(() => onCleanup(context.registerContentId(local.id!)));
 
+	onCleanup(() => context.setContentRef(undefined));
+
 	const commonAttributes: Omit<MenuContentBaseRenderProps, keyof MenuDataSet> =
 		{
 			ref: mergeRefs((el) => {

diff --git a/packages/core/src/menu/menu-context.tsx b/packages/core/src/menu/menu-context.tsx
@@ -25,7 +25,7 @@ export interface MenuContextValue {
 	triggerId: Accessor<string | undefined>;
 	contentId: Accessor<string | undefined>;
 	setTriggerRef: (el: HTMLElement) => void;
-	setContentRef: (el: HTMLElement) => void;
+	setContentRef: (el: HTMLElement | undefined) => void;
 	open: (focusStrategy: FocusStrategy | boolean) => void;
 	close: (recursively?: boolean) => void;
 	toggle: (focusStrategy: FocusStrategy | boolean) => void;

diff --git a/packages/core/src/navigation-menu/navigation-menu-root.tsx b/packages/core/src/navigation-menu/navigation-menu-root.tsx
@@ -168,6 +168,12 @@ export function NavigationMenuRoot<T extends ValidComponent = "ul">(
 		element: () => viewportRef() ?? null,
 	});
 
+	createEffect(() => {
+		if (!viewportPresent()) {
+			context.setPreviousMenu(undefined);
+		}
+	});
+
 	const context: NavigationMenuContextValue = {
 		dataset,
 		delayDuration: () => local.delayDuration!,

diff --git a/packages/core/src/navigation-menu/navigation-menu-trigger.tsx b/packages/core/src/navigation-menu/navigation-menu-trigger.tsx
@@ -69,8 +69,10 @@ export function NavigationMenuTrigger<T extends ValidComponent = "button">(
 		if (context.dataset()["data-expanded"] === "") return;
 
 		timeoutId = window.setTimeout(() => {
-			context.setAutoFocusMenu(true);
 			menuContext?.triggerRef()?.focus();
+			setTimeout(() => {
+				context.setAutoFocusMenu(true);
+			});
 		}, context.delayDuration());
 	};
 

diff --git a/packages/core/src/navigation-menu/navigation-menu-viewport.tsx b/packages/core/src/navigation-menu/navigation-menu-viewport.tsx
@@ -117,10 +117,12 @@ export function NavigationMenuViewport<T extends ValidComponent = "li">(
 	);
 
 	const height = createMemo((prev) => {
+		if (ref() === undefined || !context.viewportPresent()) return undefined;
 		if (size.height() === 0) return prev;
 		return size.height();
 	});
 	const width = createMemo((prev) => {
+		if (ref() === undefined || !context.viewportPresent()) return undefined;
 		if (size.width() === 0) return prev;
 		return size.width();
 	});

diff --git a/packages/core/src/number-field/number-field-input.tsx b/packages/core/src/number-field/number-field-input.tsx
@@ -169,7 +169,7 @@ export function NumberFieldInput<T extends ValidComponent = "input">(
 				>
 					as={local.as || "input"}
 					value={
-						Number.isNaN(context.rawValue())
+						Number.isNaN(context.rawValue()) || context.value() === undefined
 							? ""
 							: context.formatNumber(context.rawValue())
 					}

diff --git a/packages/core/src/number-field/number-field-root.tsx b/packages/core/src/number-field/number-field-root.tsx
@@ -185,6 +185,9 @@ export function NumberFieldRoot<T extends ValidComponent = "div">(
 		return new NumberFormatter(locale(), local.formatOptions);
 	});
 
+	const formatNumber = (number: number) =>
+		local.format ? numberFormatter().format(number) : number.toString();
+
 	const parseRawValue = (value: string | number | undefined) =>
 		local.format && typeof value !== "number"
 			? numberParser().parse(value ?? "")
@@ -203,14 +206,12 @@ export function NumberFieldRoot<T extends ValidComponent = "div">(
 		value: () => local.value,
 		defaultValue: () => local.defaultValue ?? local.rawValue,
 		onChange: (value) => {
-			local.onChange?.(
-				typeof value === "number" ? numberFormatter().format(value) : value,
-			);
+			local.onChange?.(typeof value === "number" ? formatNumber(value) : value);
 			local.onRawValueChange?.(parseRawValue(value));
 		},
 	});
 
-	local.onRawValueChange?.(parseRawValue(value()));
+	if (value() !== undefined) local.onRawValueChange?.(parseRawValue(value()));
 
 	function isAllowedInput(char: string): boolean {
 		if (local.allowedInput !== undefined) return local.allowedInput.test(char);
@@ -267,7 +268,7 @@ export function NumberFieldRoot<T extends ValidComponent = "div">(
 		setValue,
 		rawValue: () => parseRawValue(value()),
 		generateId: createGenerateId(() => access(formControlProps.id)!),
-		formatNumber: (number: number) => numberFormatter().format(number),
+		formatNumber,
 		format: () => {
 			if (!local.format) return;
 			let rawValue = context.rawValue();

diff --git a/packages/core/src/select/select-root.tsx b/packages/core/src/select/select-root.tsx
@@ -15,7 +15,7 @@ import {
 
 export interface SelectSingleSelectionOptions<T> {
 	/** The controlled value of the select. */
-	value?: T;
+	value?: T | null;
 
 	/**
 	 * The value of the select when initially rendered.
@@ -24,7 +24,7 @@ export interface SelectSingleSelectionOptions<T> {
 	defaultValue?: T;
 
 	/** Event handler called when the value changes. */
-	onChange?: (value: T) => void;
+	onChange?: (value: T | null) => void;
 
 	/** Whether the select allow multiple selection. */
 	multiple?: false;
@@ -101,10 +101,10 @@ export function SelectRoot<
 
 	const onChange = (value: Option[]) => {
 		if (local.multiple) {
-			local.onChange?.(value as any);
+			local.onChange?.((value ?? []) as any);
 		} else {
 			// use `null` as "no value" because `undefined` mean the component is "uncontrolled".
-			local.onChange?.((value[0] ?? null) as any);
+			local.onChange?.((value[0] ?? null) as any); // TODO: maybe return undefined? breaking change!
 		}
 	};
 

diff --git a/packages/core/src/skeleton/skeleton-root.tsx b/packages/core/src/skeleton/skeleton-root.tsx
@@ -46,8 +46,8 @@ export interface SkeletonRootCommonProps<T extends HTMLElement = HTMLElement> {
 
 export interface SkeletonRootRenderProps extends SkeletonRootCommonProps {
 	role: "group";
-	"data-animate": boolean;
-	"data-visible": boolean;
+	"data-animate": boolean | undefined;
+	"data-visible": boolean | undefined;
 }
 
 export type SkeletonRootProps<
@@ -82,8 +82,8 @@ export function Skeleton<T extends ValidComponent = "div">(
 		<Polymorphic<SkeletonRootRenderProps>
 			as="div"
 			role="group"
-			data-animate={local.animate}
-			data-visible={local.visible}
+			data-animate={local.animate || undefined}
+			data-visible={local.visible || undefined}
 			style={combineStyle(
 				{
 					"border-radius": local.circle

diff --git a/packages/core/src/text-field/text-field-root.tsx b/packages/core/src/text-field/text-field-root.tsx
@@ -1,5 +1,4 @@
 import {
-	OverrideComponentProps,
 	type ValidationState,
 	access,
 	createGenerateId,
@@ -108,8 +107,12 @@ export function TextFieldRoot<T extends ValidComponent = "div">(
 		FORM_CONTROL_PROP_NAMES,
 	);
 
+	// Disable reactivity to only track controllability on first run
+	// Our current implementation breaks with undefined (stops tracking controlled value)
+	const initialValue = local.value;
+
 	const [value, setValue] = createControllableSignal({
-		value: () => local.value,
+		value: () => (initialValue === undefined ? undefined : local.value ?? ""),
 		defaultValue: () => local.defaultValue,
 		onChange: (value) => local.onChange?.(value),
 	});