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.

Sign up for GitHub

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

fix(popover, tooltip): skip ref setup logic on component removal #11132

Merged
merged 3 commits into from
Dec 23, 2024
Merged

fix(popover, tooltip): skip ref setup logic on component removal #11132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc3624d
fix(popover, tooltip): skip ref set up logic on component removal
jcfranco Dec 22, 2024
187bbb1
add tests
jcfranco Dec 23, 2024
8885281
fix test
jcfranco Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 49 additions & 1 deletion packages/calcite-components/src/components/popover/popover.e2e.ts
View file
Open in desktop
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", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll submit a follow-up PR to clean this up and also try to add some reusable tests/utils.

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
5 changes: 4 additions & 1 deletion packages/calcite-components/src/components/popover/popover.tsx
View file
Open in desktop
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
50 changes: 49 additions & 1 deletion packages/calcite-components/src/components/tooltip/tooltip.e2e.ts
View file
Open in desktop
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
5 changes: 4 additions & 1 deletion packages/calcite-components/src/components/tooltip/tooltip.tsx
View file
Open in desktop
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
Loading