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

APP-7268: Add nav dropdown #615

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion packages/core/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@viamrobotics/prime-core",
"version": "0.0.174",
"version": "0.0.175",
"repository": {
"type": "git",
"url": "https://github.com/viamrobotics/prime.git",
Expand Down
1 change: 0 additions & 1 deletion packages/core/src/lib/__tests__/code-snippet.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ describe('CodeSnippet', () => {

const initialCode = screen.getByText(common.code);
expect(initialCode).toBeInTheDocument();

const newCode = '{ their: "json" }';
await rerender({
...common,
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export { preventHandler, preventKeyboardHandler } from './prevent-handler';

export * from './select';

export { default as NavDropdown } from './nav-dropdown/nav-dropdown.svelte';
export { default as Progress } from './progress/progress.svelte';
export { default as Switch } from './switch.svelte';
export { default as Radio } from './radio.svelte';
Expand Down
91 changes: 91 additions & 0 deletions packages/core/src/lib/nav-dropdown/__tests__/nav-dropdown.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { describe, expect, it } from 'vitest';
import { render, screen, within } from '@testing-library/svelte';
import userEvent from '@testing-library/user-event';
import { NavDropdown as Subject } from '$lib';

const versionOptions = [
{
label: 'Version 1',
detail: '1 day ago',
description: 'stable',
href: '/v1',
},
{
label: 'Version 2',
detail: '5 hours ago',
description: 'latest',
href: '/v2',
},
];

describe('NavDropdown', () => {
it('renders a button that controls a menu', () => {
render(Subject, {
props: { options: versionOptions, selectedHref: '/v1' },
});

const button = screen.getByRole('button');

expect(button).toHaveAttribute('aria-haspopup', 'menu');
expect(button).toHaveAttribute('aria-expanded', 'false');
});

it('expands the menu on click', async () => {
const user = userEvent.setup();
render(Subject, {
props: { options: versionOptions, selectedHref: '/v1' },
});

const button = screen.getByRole('button');
await user.click(button);

const menu = screen.getByRole('menu');

expect(menu).toBeInTheDocument();
expect(button).toHaveAttribute('aria-expanded', 'true');
Copy link
Member

Choose a reason for hiding this comment

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

todo: make sure that the button is marked as controlling the menu

expect(menu).toHaveAttribute('id')
expect(button).toHaveAttribute('aria-controls', menu.id)

});

it('displays option details', async () => {
const user = userEvent.setup();
render(Subject, {
props: { options: versionOptions, selectedHref: '/v1' },
});

const button = screen.getByRole('button');
await user.click(button);

const menuitem = screen.getByRole('menuitem', { name: /Version 1/u });
expect(within(menuitem).getByText(/1 day ago/u)).toBeInTheDocument();
expect(within(menuitem).getByText('stable')).toBeInTheDocument();
Comment on lines +58 to +59
Copy link
Member

Choose a reason for hiding this comment

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

question (non-blocking): Does toHaveTextContent work here rather than re-querying?

Suggested change
expect(within(menuitem).getByText(/1 day ago/u)).toBeInTheDocument();
expect(within(menuitem).getByText('stable')).toBeInTheDocument();
expect(menuitem).toHaveTextContent(/1 day ago/u);
expect(menuitem).toHaveTextContent(/stable/iu);

});

it('opens menu with Space when button is focused', async () => {
const user = userEvent.setup();
render(Subject, {
props: { options: versionOptions, selectedHref: '/v1' },
});

const button = screen.getByRole('button');
button.focus();
await user.keyboard(' ');

expect(screen.getByRole('menu')).toBeInTheDocument();
expect(button).toHaveAttribute('aria-expanded', 'true');
});

it('closes menu with Escape', async () => {
const user = userEvent.setup();
render(Subject, {
props: { options: versionOptions, selectedHref: '/v1' },
});

const button = screen.getByRole('button');
await user.click(button);
expect(screen.getByRole('menu')).toBeInTheDocument();

await user.keyboard('{Escape}');

expect(screen.queryByRole('menu')).not.toBeInTheDocument();
expect(button).toHaveAttribute('aria-expanded', 'false');
});
});
116 changes: 116 additions & 0 deletions packages/core/src/lib/nav-dropdown/nav-dropdown.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
<script lang="ts">
import { clickOutside } from '$lib/click-outside';
import { Icon } from '$lib/icon';
import { createHandleKey } from '$lib/keyboard';
import { Floating, matchWidth } from '$lib/floating';

interface NavOption {
label: string;
detail?: string;
description?: string;
href: string;
}

export let options: NavOption[] = [];
export let selectedHref: string;

let isOpen = false;
let activeIndex = -1;
Copy link
Member

Choose a reason for hiding this comment

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

issue (keyboard): when isOpen becomes true, the WAI example moves focus move automatically to the first item. Should we make sure we set activeIndex to 0 when isOpen becomes true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, makes sense, ty!

let buttonElement: HTMLButtonElement | undefined;

Copy link
Member

Choose a reason for hiding this comment

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

issue (keyboard): we want to make sure we move focus between the links when arrow keys are pressed. I think we can do that by:

  • Binding elements by index
  • Triggering focus when activeIndex changes
let linkElements = []

$: {
  if (activeIndex >= 0) {
    linkElements[activeIndex].focus();
  }
}
{#each options as { label, detail, description, href }, i}
  <a
    bind:this={linkElements[i]}
    ...

const toggleDropdown = () => {
isOpen = !isOpen;
activeIndex = isOpen ? 0 : -1;
};

const closeDropdown = () => {
isOpen = false;
};

const handleMenuItemKeydown = createHandleKey({
Escape: () => {
closeDropdown();
buttonElement?.focus();
activeIndex = -1;
},
});

const handleClickOutside = (element: Element) => {
if (!buttonElement?.contains(element)) {
closeDropdown();
}
};

$: if (isOpen) {
buttonElement?.focus();
}
</script>

<div class="group flex w-48">
<button
bind:this={buttonElement}
class="relative z-[2] h-7.5 w-full grow appearance-none border border-light bg-white py-1.5 pl-2 pr-1 text-xs leading-tight outline-none group-hover:border-gray-6"
on:click={toggleDropdown}
type="button"
aria-haspopup="menu"
aria-expanded={isOpen}
>
<div class="flex items-center justify-between">
<span class="block truncate text-xs">
{options.find((opt) => opt.href === selectedHref)?.label ??
'Latest Version'}
</span>
<Icon
name="chevron-down"
cx={['text-gray-6 transition-transform', { 'rotate-180': isOpen }]}
/>
</div>
</button>

{#if isOpen}
<Floating
referenceElement={buttonElement}
placement="bottom-start"
offset={4}
size={matchWidth}
auto
>
<div
class="w-full overflow-auto border border-gray-6 bg-white shadow-sm focus:outline-none"
role="menu"
use:clickOutside={handleClickOutside}
>
{#each options as { label, detail, description, href }, i}
<a
{href}
class="relative flex flex-col px-2 py-1.5 hover:bg-gray-1 focus:bg-gray-1 focus:outline-none"
class:bg-gray-1={i === activeIndex}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (keyboard): Should we move this styling to a focus:bg-gray-1 instead? That way, once we set up focus via keyboard events properly, it will all work

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

role="menuitem"
aria-current={href === selectedHref ? 'page' : 'false'}
on:click={closeDropdown}
on:keydown={handleMenuItemKeydown}
on:focus={() => {
activeIndex = i;
}}
on:blur={() => {
activeIndex = -1;
}}
tabindex="0"
>
<div class="flex items-center text-xs">
<span class="block truncate font-normal">{label}</span>
{#if detail}
<span class="ml-1 text-gray-6">({detail})</span>
{/if}
</div>
{#if description}
<span class="block truncate text-[0.625rem] text-gray-6"
>{description}</span
>
{/if}
</a>
{/each}
</div>
</Floating>
{/if}
</div>
39 changes: 39 additions & 0 deletions packages/core/src/routes/+page.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
CodeSnippet,
RangeInput,
Progress,
NavDropdown,
} from '$lib';
import { uniqueId } from 'lodash-es';

Expand Down Expand Up @@ -1112,6 +1113,44 @@ const onHoverDelayMsInput = (event: Event) => {
</div>
</div>

<!-- NAV Dropdown -->
<h1 class="text-2xl">NAV Dropdown</h1>
<div class="flex gap-4">
<NavDropdown
selectedHref="/v1"
options={[
{
label: 'v1',
detail: '1 day ago',
description: 'stable',
href: '/v1',
},
{
label: 'v2',
detail: '5 hours ago',
description: 'latest',
href: '/v2',
},
{
label: 'v3',
detail: '2 weeks ago',
href: '/v3',
},
{
label: 'v4',
detail: '1 month ago',
href: '/v4',
},
{
label: 'v5',
detail: '2 months ago',
description: 'legacy',
href: '/v5',
},
]}
/>
</div>

<!-- Notify -->
<h1 class="text-2xl">Notify</h1>

Expand Down
Loading
Loading