Repository: melt-ui/melt-ui
Pull request #716 (2023-11-09 01:49:47)

Fix: Menubar Menu BYOID Issues
Also adds a few more tests to keep chipping away at them 🫡

changeset-bot[bot]: ###  ⚠️  No Changeset found

Latest commit: 1ddb362be5f8f29d9189a701d3dc816d53231210

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. **If these changes should result in a version bump, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/huntabyte/melt-ui/new/fix/menubar-ids?filename=.changeset/good-hats-happen.md&value=---%0A%22%40melt-ui%2Fsvelte%22%3A%20patch%0A---%0A%0AFix%3A%20Slider%20%26%20Menubar%20Menu%20BYOID%20Issues%0A)



Patch:
diff --git a/.changeset/twenty-lemons-sneeze.md b/.changeset/twenty-lemons-sneeze.md
new file mode 100644
index 00000000..0c8b0533
--- /dev/null
+++ b/.changeset/twenty-lemons-sneeze.md
@@ -0,0 +1,5 @@
+---
+"@melt-ui/svelte": patch
+---
+
+Fix: Menubar Menu bring your own id 
diff --git a/src/lib/builders/menubar/create.ts b/src/lib/builders/menubar/create.ts
index 117c4b41..6ee7ad53 100644
--- a/src/lib/builders/menubar/create.ts
+++ b/src/lib/builders/menubar/create.ts
@@ -398,7 +398,7 @@ export function createMenubar(props?: CreateMenubarProps) {
 		});
 
 		return {
-			ids,
+			ids: m.ids,
 			elements: {
 				menu,
 				trigger,
diff --git a/src/lib/builders/slider/create.ts b/src/lib/builders/slider/create.ts
index a0e33aed..343d3d79 100644
--- a/src/lib/builders/slider/create.ts
+++ b/src/lib/builders/slider/create.ts
@@ -48,7 +48,7 @@ export const createSlider = (props?: CreateSliderProps) => {
 	const currentThumbIndex = writable<number>(0);
 	const activeThumb = writable<{ thumb: HTMLElement; index: number } | null>(null);
 
-	const meltIds = generateIds(['root']);
+	const meltIds = generateIds(['root'] as const);
 
 	// Helpers
 	const updatePosition = (val: number, index: number) => {
@@ -464,7 +464,6 @@ export const createSlider = (props?: CreateSliderProps) => {
 	});
 
 	return {
-		ids: meltIds,
 		elements: {
 			root,
 			thumb,


Test patch:
diff --git a/src/tests/collapsible/Collapsible.spec.ts b/src/tests/collapsible/Collapsible.spec.ts
index d37efc61..752c94a4 100644
--- a/src/tests/collapsible/Collapsible.spec.ts
+++ b/src/tests/collapsible/Collapsible.spec.ts
@@ -3,6 +3,18 @@ import { axe } from 'jest-axe';
 import { describe } from 'vitest';
 import CollapsibleTest from './CollapsibleTest.svelte';
 import userEvent from '@testing-library/user-event';
+import type { CreateCollapsibleProps } from '$lib';
+import { get, writable } from 'svelte/store';
+import { sleep, type ChangeFn } from '$lib/internal/helpers';
+
+function setup(props: CreateCollapsibleProps = {}) {
+	const user = userEvent.setup();
+	const returned = render(CollapsibleTest, props);
+	return {
+		...returned,
+		user,
+	};
+}
 
 describe('Collapsible', () => {
 	test('No accessibility violations', async () => {
@@ -12,8 +24,7 @@ describe('Collapsible', () => {
 	});
 
 	test('Toggles when clicked', async () => {
-		const user = userEvent.setup();
-		const { getByTestId } = render(CollapsibleTest);
+		const { getByTestId, user } = setup();
 
 		const trigger = getByTestId('trigger');
 
@@ -27,12 +38,43 @@ describe('Collapsible', () => {
 	});
 
 	test('Should be open when open prop is true', async () => {
-		const { getByTestId } = render(CollapsibleTest, { open: true });
+		const { getByTestId } = setup({ defaultOpen: true });
 		expect(getByTestId('content')).toBeVisible();
 	});
 
 	test('Should be closed when open prop is false', async () => {
-		const { getByTestId } = render(CollapsibleTest, { open: false });
+		const { getByTestId } = setup({ defaultOpen: false });
+		expect(getByTestId('content')).not.toBeVisible();
+	});
+
+	test('Correctly handles controlled store updates', async () => {
+		const customOpen = writable(false);
+
+		const { getByTestId, user } = setup({ open: customOpen });
+		expect(getByTestId('content')).not.toBeVisible();
+
+		customOpen.set(true);
+		await sleep(1);
+		expect(getByTestId('content')).toBeVisible();
+		await user.click(getByTestId('trigger'));
+		await sleep(1);
+		expect(get(customOpen)).toBe(false);
+	});
+
+	test('Should be disabled when disabled prop is true', async () => {
+		const { getByTestId, user } = setup({ disabled: true });
+		expect(getByTestId('content')).not.toBeVisible();
+		await user.click(getByTestId('trigger'));
+		expect(getByTestId('content')).not.toBeVisible();
+	});
+
+	test('Respects `onOpenChange` callback when the open state changes', async () => {
+		const onOpenChange: ChangeFn<boolean> = () => {
+			return false;
+		};
+		const { getByTestId, user } = setup({ onOpenChange });
+		expect(getByTestId('content')).not.toBeVisible();
+		await user.click(getByTestId('trigger'));
 		expect(getByTestId('content')).not.toBeVisible();
 	});
 });
diff --git a/src/tests/collapsible/CollapsibleTest.svelte b/src/tests/collapsible/CollapsibleTest.svelte
index 923689e9..d6bb8f9d 100644
--- a/src/tests/collapsible/CollapsibleTest.svelte
+++ b/src/tests/collapsible/CollapsibleTest.svelte
@@ -1,12 +1,12 @@
 <script lang="ts">
-	import { createCollapsible, melt } from '$lib/index.js';
+	import { createCollapsible, melt, type CreateCollapsibleProps } from '$lib/index.js';
 
-	export let open = false;
+	type $$Props = CreateCollapsibleProps;
 
 	const {
 		elements: { root, trigger, content },
 	} = createCollapsible({
-		defaultOpen: open,
+		...$$restProps,
 	});
 </script>
 
diff --git a/src/tests/context-menu/ContextMenu.spec.ts b/src/tests/context-menu/ContextMenu.spec.ts
index bc6cddbe..acf24a45 100644
--- a/src/tests/context-menu/ContextMenu.spec.ts
+++ b/src/tests/context-menu/ContextMenu.spec.ts
@@ -1,6 +1,6 @@
 import { render, waitFor } from '@testing-library/svelte';
 import { axe } from 'jest-axe';
-import { describe } from 'vitest';
+import { describe, vi } from 'vitest';
 import userEvent from '@testing-library/user-event';
 import { testKbd as kbd } from '../utils.js';
 import ContextMenuTest from './ContextMenuTest.svelte';
@@ -34,6 +34,16 @@ async function open(props: CreateContextMenuProps = {}) {
 }
 
 describe('Context Menu', () => {
+	beforeEach(() => {
+		vi.stubGlobal('requestAnimationFrame', (fn: FrameRequestCallback) => {
+			return window.setTimeout(() => fn(Date.now()), 16);
+		});
+	});
+
+	afterEach(() => {
+		vi.unstubAllGlobals();
+	});
+
 	test('No accessibility violations', async () => {
 		const { container } = render(ContextMenuTest);
 
@@ -132,6 +142,19 @@ describe('Context Menu', () => {
 		expect(subItem1).toHaveFocus();
 	});
 
+	test('Should close on outside click by default', async () => {
+		const { user, getByTestId, queryByTestId } = await open();
+		const outsideClick = getByTestId('outside-click');
+		await user.click(outsideClick);
+		await waitFor(() => expect(queryByTestId('menu')).toBeNull());
+	});
+
+	test('Should close on escape', async () => {
+		const { user, queryByTestId } = await open();
+		await user.keyboard(kbd.ESCAPE);
+		await waitFor(() => expect(queryByTestId('menu')).toBeNull());
+	});
+
 	test('respects `closeOnEscape` prop', async () => {
 		const { user, getByTestId } = await open({
 			closeOnEscape: false,
diff --git a/src/tests/dialog/DialogTest.svelte b/src/tests/dialog/DialogTest.svelte
index 815e1dd0..f22dfc22 100644
--- a/src/tests/dialog/DialogTest.svelte
+++ b/src/tests/dialog/DialogTest.svelte
@@ -3,14 +3,9 @@
 
 	type $$Props = CreateDialogProps;
 
-	export let closeFocus: CreateDialogProps['closeFocus'] = undefined;
-	export let openFocus: CreateDialogProps['openFocus'] = undefined;
-
 	const {
 		elements: { trigger, overlay, content, title, description, close, portalled },
 	} = createDialog({
-		closeFocus,
-		openFocus,
 		...$$restProps,
 	});
 </script>