feat(atomic): migrate atomic-popover from Stencil to Lit

[Copilot]

✅ Checklist

• 🧪 The component is unit tested
• 🧪 The component includes E2E tests*
• 🗑️ Old Cypress tests exclusive to the component are removed~ (N/A)
• 📖 The component is documented in storybook with an .mdx file
• ♿ The component complies with the Web Content Accessibility Guidelines.
• 🌐 All strings intended for humans or assistive technology must be localized with i18n.
• 📦 The Lit component is exported in the appropriate index.ts and lazy-index.ts files.
• 🎨 CSS parts are documented still accessible.
• 🦥 Slotted Content, public methods and properties are documented
• 🔄 The component outputs the same Angular output as before with Stencil
• 🏷️ The component declares the component type in the HTMLElementTagNameMap

https://coveord.atlassian.net/browse/KIT-4953

[jmazzah-coveo]

@copilot Have describe blocks with method names ex describe #render and test them in order. Also, bring the render method above the private methods

[Copilot]

Done! Tests are now organized with method-based describe blocks (#initialize, #render, #disconnectedCallback, #togglePopover), and the component's render method has been moved above the private methods.

Changes in commit aa2676f.

[jmazzah-coveo]

@copilot have a describe with private method, test them throught the public methods that call them, and put the test in the describe of the public method

[Copilot]

Reorganized tests to remove the #togglePopover describe block (private method). The private togglePopover() method is now tested through the public render method's interactions:

• when button is clicked - tests button interaction
• when backdrop is clicked - tests backdrop interaction
• when Escape key is pressed - tests keyboard interaction

All tests remain in the #render describe block since that's where these user interactions are rendered.

Changes in commit fe3074a.

[Copilot]

Pull request overview

This PR migrates the atomic-popover component from Stencil to Lit, following the established migration pattern for Atomic components. The component displays a single facet as a popover menu with a button trigger, backdrop, and keyboard support.

Key changes:

• Converted component from Stencil (.tsx) to Lit (.ts) with Tailwind styling
• Migrated from Stencil lifecycle methods to Lit lifecycle callbacks
• Updated from Stencil's @Component, @State, @Listen to Lit's @customElement, @state, and event listeners in connectedCallback
• Replaced functional button helper with renderButton functional component
• Added comprehensive unit tests using Vitest browser mode
• Added E2E tests using Playwright with page object pattern
• Created Storybook documentation with MSW API mocking
• Removed legacy Cypress test suite

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
atomic-popover.tsx (removed)	Removed original Stencil implementation
atomic-popover.pcss (removed)	Removed PostCSS styles (now using Tailwind)
atomic-popover.ts (new)	New Lit component implementation with Tailwind styles
atomic-popover.spec.ts (new)	Vitest unit tests with browser mode assertions
atomic-popover.new.stories.tsx (new)	Storybook stories with MSW API mocking for facet data
atomic-popover.mdx (new)	Component documentation for Storybook
e2e/atomic-popover.e2e.ts (new)	Playwright E2E tests for user interactions
e2e/page-object.ts (new)	Page object pattern for E2E test selectors
e2e/fixture.ts (new)	Test fixture setup for E2E tests
cypress/e2e/facets/popover-facet/* (removed)	Removed legacy Cypress test suite (4 files)
components.d.ts	Auto-generated type definitions updated to remove Stencil types

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[Copilot]

The beforeEach hook calls searchApiHarness.searchEndpoint.clear() but this is unnecessary. According to MSW API mocking guidelines, you only need to call clear() when you have queued responses with mockOnce() that you want to flush. Since this story file doesn't use mockOnce() anywhere, and the base mock at lines 10-58 uses mock() (which is permanent), the clear() call doesn't do anything and should be removed.

[Copilot]

Missing test coverage for the error case when more than one child is provided. The component's initialize() method (lines 85-89 of atomic-popover.ts) validates this condition and sets an error, but there's no test verifying this behavior.

Add a test similar to the existing "when no child is provided" test:

describe('when more than one child is provided', () => {
  it('should set error', async () => {
    const element = (await renderInAtomicSearchInterface<AtomicPopover>({
      template: html`<atomic-popover>
        <div>Child 1</div>
        <div>Child 2</div>
      </atomic-popover>`,
      selector: 'atomic-popover',
      bindings: (bindings) => {
        bindings.engine = mockEngine;
        return bindings;
      },
    })).element;

    expect(element.error).toBeTruthy();
    expect(element.error.message).toContain(
      'Cannot have more than one child inside a set of popover tags.'
    );
  });
});

[Copilot]

The documentation description says "displays facets" (plural) but the component only supports a single facet child. The validation in initialize() (lines 77-89 of atomic-popover.ts) enforces exactly one child. The description should be changed to "displays a facet" (singular) for accuracy.

Also, the phrase "one or more facets" at line 15 is incorrect and should be "a single facet" or just "a facet".

Suggested change

	The `atomic-popover` component displays facets as compact popover menus. When a user clicks the popover button, a backdrop appears along with a popover containing one or more facets.
	The `atomic-popover` component displays a facet as a compact popover menu. When a user clicks the popover button, a backdrop appears along with a popover containing a single facet.

[Copilot]

Missing accessibility tests. According to E2E testing guidelines, components should include accessibility testing. The popover component has several accessibility features (aria-expanded, aria-controls, focus trap, keyboard navigation) that should be verified.

Consider adding tests for:

• aria-expanded attribute correctly reflects open/closed state
• aria-controls points to the correct popover ID
• Keyboard focus management with the focus trap
• Proper role and labeling of the button
• Tab order when popover is open vs closed

You could add a test using toMatchAriaSnapshot to verify the accessibility tree structure.

[Copilot]

The method name toggleOpen() is inconsistent with naming conventions used elsewhere in the codebase. In the Stencil version (line 107 of the removed file), this was togglePopover(), which better describes what it does. Naming should use a verb that clearly indicates the action being performed. Consider renaming to togglePopover() for better clarity and consistency with the original implementation, or at least to toggle() to match the pattern of other similar methods.

Suggested change

	private toggleOpen() {
	private togglePopover() {

[Copilot]

The test should use test.step() to group the interactions into logical steps. According to Playwright test writing guidelines, using test.step() improves test readability and reporting by grouping related interactions.

For example, the test at lines 9-18 could be:

test('should open and close popover with button', async ({popover}) => {
  await test.step('Verify initial closed state', async () => {
    await expect(popover.facetContainer).not.toBeVisible();
  });

  await test.step('Open popover', async () => {
    await popover.popoverButton.click();
    await expect(popover.facetContainer).toBeVisible();
    await expect(popover.backdrop).toBeVisible();
  });

  await test.step('Close popover', async () => {
    await popover.popoverButton.click();
    await expect(popover.facetContainer).not.toBeVisible();
  });
});

[Copilot]

[nitpick] The comment "Wait for component to be stable before screenshot" is misleading. The method doesn't take a screenshot—it only prepares for one by waiting for stability. The captureScreenshot() method below (line 50) is what actually takes the screenshot. Consider renaming this method to waitForStability() or updating the comment to say "Wait for component to be stable (useful before taking screenshots or assertions)".

[Copilot]

Missing test coverage for the handleInitializePopover event handler logic. While the event is dispatched in the renderPopover helper (lines 90-96), there's no test verifying:

1. That the childFacet is set when the event is received
2. That the popoverClass is added to the child element (line 250 of atomic-popover.ts)
3. That subsequent events are ignored when childFacet is already set (line 245 of atomic-popover.ts)
4. That events without detail are ignored (line 245 of atomic-popover.ts)

Add tests in a new describe('#handleInitializePopover') block to verify this behavior.

[Copilot]

Missing test for closing the popover by clicking the backdrop. The component supports closing the popover by clicking the backdrop (line 223 of atomic-popover.ts), but this interaction is not tested in the E2E tests. Add a test case:

test('should close popover when clicking backdrop', async ({popover}) => {
  await test.step('Open popover', async () => {
    await popover.popoverButton.click();
    await expect(popover.facetContainer).toBeVisible();
  });

  await test.step('Close by clicking backdrop', async () => {
    await popover.backdrop.click();
    await expect(popover.facetContainer).not.toBeVisible();
  });
});

[Copilot]

The backdrop locator uses getByTestId('backdrop'), but the component doesn't have a data-testid="backdrop" attribute. The backdrop element only has part="backdrop" (line 221 of atomic-popover.ts). This locator will fail to find the element.

Use a part-based selector instead, similar to the parts object pattern defined below. The locator is also never used in the tests, so it should either be removed or used in tests that currently query the backdrop directly via parts(element).backdrop.

Suggested change

	get backdrop() {
	return page.getByTestId('backdrop');
	},