Skip to content

Commit

Permalink
Merge pull request #716 from thejackshelton/fix-popover
Browse files Browse the repository at this point in the history
refactor: improved popover middle tests
  • Loading branch information
thejackshelton authored Apr 22, 2024
2 parents 5481490 + 1ff2d38 commit 51d1a7f
Show file tree
Hide file tree
Showing 6 changed files with 150 additions and 107 deletions.
5 changes: 5 additions & 0 deletions .changeset/blue-spiders-flash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@qwik-ui/headless': patch
---

fix: race condition in the popover when programmatically opening on supported browsers
14 changes: 14 additions & 0 deletions apps/website/src/routes/docs/headless/popover/examples/basic.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { component$ } from '@builder.io/qwik';
import { Popover, PopoverTrigger } from '@qwik-ui/headless';
export default component$(() => {
return (
<>
<PopoverTrigger popovertarget="hero-id" class="popover-trigger">
Popover Trigger
</PopoverTrigger>
<Popover id="hero-id" class="popover">
My Hero!
</Popover>
</>
);
});
19 changes: 14 additions & 5 deletions apps/website/src/routes/docs/headless/popover/examples/hero.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
import { component$ } from '@builder.io/qwik';
import { component$, useSignal } from '@builder.io/qwik';
import { Popover, PopoverTrigger } from '@qwik-ui/headless';

export default component$(() => {
const triggerRef = useSignal<HTMLButtonElement>();

return (
<>
<PopoverTrigger popovertarget="hero-id" class="popover-trigger">
Popover Trigger
<PopoverTrigger ref={triggerRef} popovertarget="popover-id" class="popover-trigger">
Click me
</PopoverTrigger>
<Popover id="hero-id" class="popover">
My Hero!
<Popover
anchorRef={triggerRef}
floating={true}
gutter={4}
id="popover-id"
class="popover"
>
I am anchored to the popover trigger!
</Popover>
</>
);
Expand Down
14 changes: 12 additions & 2 deletions apps/website/src/routes/docs/headless/popover/index.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,17 @@ A non-modal primitive with overlays that open and close around a DOM element.
]}
/>

<Note status="warning">
You are about to see a fixed scrolling popover. Yes, this behavior is intended. It is
part of the native popover API! If you want to anchor items, there is a floating section
that covers this behavior.
</Note>

<Showcase name="basic" />

The Qwik UI Popover component is designed to align with the [HTML Popover API Specification](https://developer.mozilla.org/en-US/docs/Web/API/Popover_API). As of now, native support for this API is around `82.5%`, and in almost every browser.

To ensure consistent behavior across all browsers, Qwik UI provides a `polyfill` for browser versions that do not support the API natively.
To ensure consistent behavior across all browsers, Qwik UI provides a `polyfill` with feature parity for browser versions that do not support the API natively.

<div class="flex flex-col gap-2">
<div>
Expand Down Expand Up @@ -296,7 +304,9 @@ useTask$(async ({ track }) => {

To use the popover API with floating elements, you can add the `floating={true}` prop to the Popover component. This API enables the use of JavaScript to dynamically position the listbox using the `top` & `left` absolute properties.

> Enabling the `floating={true}` property will introduce a slight increase in code size, as we currently utilize JavaScript to implement floating items. We've strived to keep it as minimal as possible, but powerful in building complex components in anticipation of the forthcoming anchor API.
Enabling the `floating={true}` property will introduce a slight increase in code size, as we currently utilize JavaScript to implement floating items. We've strived to keep it as minimal as possible, but powerful in building complex components.

> Once the native anchor API is available in all browsers, you can remove the floating prop and use CSS instead! Smaller bundle size for the win!
To float an element, it must have an anchored element to latch onto. This can be done with the `anchorRef` prop.

Expand Down
20 changes: 19 additions & 1 deletion packages/kit-headless/src/components/popover/popover.driver.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { type Locator, type Page } from '@playwright/test';
import { expect, type Locator, type Page } from '@playwright/test';

export type DriverLocator = Locator | Page;

export type PopoverOpenKeys = 'Enter' | 'Space';

export function createTestDriver<T extends DriverLocator>(rootLocator: T) {
const getPopover = () => {
return rootLocator.locator('[popover]');
Expand All @@ -11,6 +13,21 @@ export function createTestDriver<T extends DriverLocator>(rootLocator: T) {
return rootLocator.locator('[popovertarget]');
};

const openPopover = async (key: PopoverOpenKeys | 'click', index?: number) => {
const action = key === 'click' ? 'click' : 'press';
const trigger = index !== undefined ? getTrigger().nth(index) : getTrigger();
const popover = index !== undefined ? getPopover().nth(index) : getPopover();

if (action === 'click') {
await trigger.click({ position: { x: 1, y: 1 } }); // Modified line
} else {
await trigger.press(key);
}

// Needed because Playwright doesn't wait for the listbox to be visible
await expect(popover).toBeVisible();
};

const getAllPopovers = () => {
return getPopover().all();
};
Expand All @@ -30,6 +47,7 @@ export function createTestDriver<T extends DriverLocator>(rootLocator: T) {
getAllPopovers,
getTrigger,
getAllTriggers,
openPopover,
getProgrammaticButtonTrigger,
};
}
Loading

0 comments on commit 51d1a7f

Please sign in to comment.