Skip to content

Commit

Permalink
fix(popover, tooltip): skip ref setup logic on component removal (#11132
Browse files Browse the repository at this point in the history
)

**Related Issue:** #10731

## Summary

Adds a guard to `popover` and `tooltip` `ref` callbacks to avoid missing
ref warnings on removal. After #10310, `ref` callbacks are invoked both
when the component is added and removed (see
#11093).

BEGIN_COMMIT_OVERRIDE
omitted from changelog
END_COMMIT_OVERRIDE
  • Loading branch information
jcfranco authored Dec 23, 2024
1 parent d1f6f1d commit e67b1fe
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 4 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { newE2EPage } from "@arcgis/lumina-compiler/puppeteerTesting";
import { describe, expect, it } from "vitest";
import { afterEach, beforeEach, describe, expect, it, MockInstance, vi } from "vitest";
import { html } from "../../../support/formatting";
import {
accessible,
Expand Down Expand Up @@ -729,6 +729,54 @@ describe("calcite-popover", () => {
});
});

describe("warning messages", () => {
let consoleSpy: MockInstance;

beforeEach(
() =>
(consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {
// hide warning messages during test
})),
);

afterEach(() => consoleSpy.mockClear());

it("does not warn if reference element is present", async () => {
const page = await newE2EPage();
await page.setContent(
html`<calcite-popover reference-element="ref">content</calcite-popover>
<div id="ref">referenceElement</div>`,
);
await page.waitForChanges();

expect(consoleSpy).not.toHaveBeenCalled();
});

it("does not warn after removal", async () => {
const page = await newE2EPage();
await page.setContent(
html`<calcite-popover reference-element="ref">content</calcite-popover>
<div id="ref">referenceElement</div>`,
);
await page.waitForChanges();
const popover = await page.find("calcite-popover");
await popover.callMethod("remove");
await page.waitForChanges();

expect(consoleSpy).not.toHaveBeenCalled();
});

it("warns if reference element is not present", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-popover reference-element="non-existent-ref">content</calcite-popover>`);
await page.waitForChanges();

expect(consoleSpy).toHaveBeenCalledWith(
expect.stringMatching(new RegExp(`reference-element id "non-existent-ref" was not found`)),
);
});
});

describe("theme", () => {
describe("default", () => {
themed(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,10 @@ export class Popover

private setFloatingEl(el: HTMLDivElement): void {
this.floatingEl = el;
requestAnimationFrame(() => this.setUpReferenceElement());

if (el) {
requestAnimationFrame(() => this.setUpReferenceElement());
}
}

private setTransitionEl(el: HTMLDivElement): void {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { newE2EPage, E2EPage } from "@arcgis/lumina-compiler/puppeteerTesting";
import { describe, expect, it } from "vitest";
import { afterEach, beforeEach, describe, expect, it, MockInstance, vi } from "vitest";
import { accessible, defaults, floatingUIOwner, hidden, openClose, renders, themed } from "../../tests/commonTests";
import { html } from "../../../support/formatting";
import { getElementXY, GlobalTestProps, skipAnimations } from "../../tests/utils";
Expand Down Expand Up @@ -1204,6 +1204,54 @@ describe("calcite-tooltip", () => {
});
});

describe("warning messages", () => {
let consoleSpy: MockInstance;

beforeEach(
() =>
(consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {
// hide warning messages during test
})),
);

afterEach(() => consoleSpy.mockClear());

it("does not warn if reference element is present", async () => {
const page = await newE2EPage();
await page.setContent(
html` <calcite-tooltip reference-element="ref">content</calcite-tooltip>
<div id="ref">referenceElement</div>`,
);
await page.waitForChanges();

expect(consoleSpy).not.toHaveBeenCalled();
});

it("does not warn after removal", async () => {
const page = await newE2EPage();
await page.setContent(
html` <calcite-tooltip reference-element="ref">content</calcite-tooltip>
<div id="ref">referenceElement</div>`,
);
await page.waitForChanges();
const tooltip = await page.find("calcite-tooltip");
await tooltip.callMethod("remove");
await page.waitForChanges();

expect(consoleSpy).not.toHaveBeenCalled();
});

it("warns if reference element is not present", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-tooltip reference-element="non-existent-ref">content</calcite-tooltip>`);
await page.waitForChanges();

expect(consoleSpy).toHaveBeenCalledWith(
expect.stringMatching(new RegExp(`reference-element id "non-existent-ref" was not found`)),
);
});
});

describe("theme", () => {
describe("default", () => {
themed(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,10 @@ export class Tooltip extends LitElement implements FloatingUIComponent, OpenClos

private setFloatingEl(el: HTMLDivElement): void {
this.floatingEl = el;
requestAnimationFrame(() => this.setUpReferenceElement());

if (el) {
requestAnimationFrame(() => this.setUpReferenceElement());
}
}

private setTransitionEl(el: HTMLDivElement): void {
Expand Down

0 comments on commit e67b1fe

Please sign in to comment.