From 2e2123248f2018430cbb09091270add44dbd06a4 Mon Sep 17 00:00:00 2001 From: Jose Carcamo Date: Fri, 7 Feb 2025 11:01:31 -0800 Subject: [PATCH 1/6] fix(tabs): x redisplays when > 1 closable tab --- .../src/components/tab-nav/tab-nav.tsx | 12 +++++++++++ .../src/components/tabs/tabs.e2e.ts | 21 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx index ee20703666d..dbe92cfa53f 100644 --- a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx +++ b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx @@ -129,6 +129,13 @@ export class TabNav extends LitElement { /** Specifies text to update multiple components to keep in sync if one changes. */ @property({ reflect: true }) syncId: string; + /** + * Keeps track whether the first tab title was ever closable + * + * @private + */ + firstTabClosable = false; + // #endregion // #region Events @@ -374,6 +381,10 @@ export class TabNav extends LitElement { slottedElements.forEach((child) => { this.intersectionObserver?.observe(child); }); + if (slottedElements.length > 1 && this.firstTabClosable) { + slottedElements[0].closable = true; + } + this.calciteInternalTabNavSlotChange.emit(slottedElements); } @@ -529,6 +540,7 @@ export class TabNav extends LitElement { const totalVisibleTabTitles = visibleTabTitlesIndices.length; if (totalVisibleTabTitles === 1 && tabTitles[visibleTabTitlesIndices[0]].closable) { + this.firstTabClosable = true; tabTitles[visibleTabTitlesIndices[0]].closable = false; this.selectedTabId = visibleTabTitlesIndices[0]; diff --git a/packages/calcite-components/src/components/tabs/tabs.e2e.ts b/packages/calcite-components/src/components/tabs/tabs.e2e.ts index 1ad96997de1..65587b89eba 100644 --- a/packages/calcite-components/src/components/tabs/tabs.e2e.ts +++ b/packages/calcite-components/src/components/tabs/tabs.e2e.ts @@ -405,5 +405,26 @@ describe("calcite-tabs", () => { expect(selectedTitleOnEmit).toBe("Tab 2 Title"); }); + + it("should hide x when last closable tab and display x when > 1 closable tabs", async () => { + expect(await allTabTitles[0].getProperty("closable")).toBe(true); + expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeDefined(); + for (let i = 2; i <= 4; ++i) { + await page.click(`#tab-title-${i} >>> .${TabTitleCSS.closeButton}`); + } + let tab1 = await page.find(`#tab-title-1`); + expect(await tab1.getProperty("closable")).toBe(false); + expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeNull(); + + await page.$eval("calcite-tabs", (element: Tabs["el"]) => { + element.ownerDocument + .getElementById("tab-title-4") + .insertAdjacentHTML("afterend", `Test`); + }); + await page.waitForChanges(); + tab1 = await page.find(`#tab-title-1`); + expect(await tab1.getProperty("closable")).toBe(true); + expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeDefined(); + }); }); }); From 1f233230230b57f0a2ecbc14650efdbdcc9b4d44 Mon Sep 17 00:00:00 2001 From: Jose Carcamo Date: Fri, 7 Feb 2025 15:46:19 -0800 Subject: [PATCH 2/6] Fixed runtime error --- packages/calcite-components/src/components/tab-nav/tab-nav.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx index dbe92cfa53f..5cd7112e9be 100644 --- a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx +++ b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx @@ -382,7 +382,7 @@ export class TabNav extends LitElement { this.intersectionObserver?.observe(child); }); if (slottedElements.length > 1 && this.firstTabClosable) { - slottedElements[0].closable = true; + (slottedElements[0] as TabTitle["el"]).closable = true; } this.calciteInternalTabNavSlotChange.emit(slottedElements); From f9bb80046cae4217d65d1ef570b70a32ea0750ca Mon Sep 17 00:00:00 2001 From: Jose Carcamo Date: Tue, 11 Feb 2025 15:33:41 -0800 Subject: [PATCH 3/6] Made firstTabClosable private; casted return type; used document object directly --- .../src/components/tab-nav/tab-nav.tsx | 16 +++++++--------- .../src/components/tabs/tabs.e2e.ts | 6 ++---- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx index 5cd7112e9be..f5129f2ca79 100644 --- a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx +++ b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx @@ -72,6 +72,8 @@ export class TabNav extends LitElement { return filterDirectChildren(this.el, "calcite-tab-title"); } + private firstTabClosable = false; + // #endregion // #region State Properties @@ -129,13 +131,6 @@ export class TabNav extends LitElement { /** Specifies text to update multiple components to keep in sync if one changes. */ @property({ reflect: true }) syncId: string; - /** - * Keeps track whether the first tab title was ever closable - * - * @private - */ - firstTabClosable = false; - // #endregion // #region Events @@ -377,12 +372,15 @@ export class TabNav extends LitElement { private onSlotChange(event: Event): void { this.intersectionObserver?.disconnect(); - const slottedElements = slotChangeGetAssignedElements(event, "calcite-tab-title"); + const slottedElements = slotChangeGetAssignedElements( + event, + "calcite-tab-title", + ); slottedElements.forEach((child) => { this.intersectionObserver?.observe(child); }); if (slottedElements.length > 1 && this.firstTabClosable) { - (slottedElements[0] as TabTitle["el"]).closable = true; + slottedElements[0].closable = true; } this.calciteInternalTabNavSlotChange.emit(slottedElements); diff --git a/packages/calcite-components/src/components/tabs/tabs.e2e.ts b/packages/calcite-components/src/components/tabs/tabs.e2e.ts index 65587b89eba..2250130cfb3 100644 --- a/packages/calcite-components/src/components/tabs/tabs.e2e.ts +++ b/packages/calcite-components/src/components/tabs/tabs.e2e.ts @@ -407,8 +407,6 @@ describe("calcite-tabs", () => { }); it("should hide x when last closable tab and display x when > 1 closable tabs", async () => { - expect(await allTabTitles[0].getProperty("closable")).toBe(true); - expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeDefined(); for (let i = 2; i <= 4; ++i) { await page.click(`#tab-title-${i} >>> .${TabTitleCSS.closeButton}`); } @@ -416,8 +414,8 @@ describe("calcite-tabs", () => { expect(await tab1.getProperty("closable")).toBe(false); expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeNull(); - await page.$eval("calcite-tabs", (element: Tabs["el"]) => { - element.ownerDocument + await page.evaluate(() => { + document .getElementById("tab-title-4") .insertAdjacentHTML("afterend", `Test`); }); From c814f6dfd4c9c05ac7d15b1a95b118a97eae6555 Mon Sep 17 00:00:00 2001 From: Jose Carcamo Date: Tue, 11 Feb 2025 17:13:47 -0800 Subject: [PATCH 4/6] Fixed code to correctly detect whether the first tab should be displayed closable --- .../src/components/tab-nav/tab-nav.tsx | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx index f5129f2ca79..e39531e5a12 100644 --- a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx +++ b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx @@ -72,7 +72,7 @@ export class TabNav extends LitElement { return filterDirectChildren(this.el, "calcite-tab-title"); } - private firstTabClosable = false; + private firstVisibleTabClosable = false; // #endregion @@ -379,8 +379,11 @@ export class TabNav extends LitElement { slottedElements.forEach((child) => { this.intersectionObserver?.observe(child); }); - if (slottedElements.length > 1 && this.firstTabClosable) { - slottedElements[0].closable = true; + const visibleTabTitlesIndices = this.visibleTabTitlesIndices; + const totalVisibleTabTitles = visibleTabTitlesIndices.length; + if (totalVisibleTabTitles > 1 && this.firstVisibleTabWasClosable) { + slottedElements[visibleTabTitlesIndices[0]].closable = true; + this.firstVisibleTabWasClosable = false; } this.calciteInternalTabNavSlotChange.emit(slottedElements); @@ -526,19 +529,23 @@ export class TabNav extends LitElement { }); } - private handleTabTitleClose(closedTabTitleEl: TabTitle["el"]): void { - const { tabTitles } = this; - const selectionModified = closedTabTitleEl.selected; - - const visibleTabTitlesIndices = tabTitles.reduce( + private get visibleTabTitlesIndices(): number[] { + return this.tabTitles.reduce( (tabTitleIndices: number[], tabTitle, index) => !tabTitle.closed ? [...tabTitleIndices, index] : tabTitleIndices, [], ); + } + + private handleTabTitleClose(closedTabTitleEl: TabTitle["el"]): void { + const { tabTitles } = this; + const selectionModified = closedTabTitleEl.selected; + + const visibleTabTitlesIndices = this.visibleTabTitlesIndices; const totalVisibleTabTitles = visibleTabTitlesIndices.length; if (totalVisibleTabTitles === 1 && tabTitles[visibleTabTitlesIndices[0]].closable) { - this.firstTabClosable = true; + this.firstVisibleTabWasClosable = true; tabTitles[visibleTabTitlesIndices[0]].closable = false; this.selectedTabId = visibleTabTitlesIndices[0]; From 6b38326fe1130c551461c7fd6fef35630a449844 Mon Sep 17 00:00:00 2001 From: Jose Carcamo Date: Wed, 12 Feb 2025 12:15:11 -0800 Subject: [PATCH 5/6] Changed variable name --- .../calcite-components/src/components/tab-nav/tab-nav.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx index e39531e5a12..34678e7fcee 100644 --- a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx +++ b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx @@ -72,7 +72,7 @@ export class TabNav extends LitElement { return filterDirectChildren(this.el, "calcite-tab-title"); } - private firstVisibleTabClosable = false; + private makeFirstVisibleTabClosable = false; // #endregion @@ -381,9 +381,9 @@ export class TabNav extends LitElement { }); const visibleTabTitlesIndices = this.visibleTabTitlesIndices; const totalVisibleTabTitles = visibleTabTitlesIndices.length; - if (totalVisibleTabTitles > 1 && this.firstVisibleTabWasClosable) { + if (totalVisibleTabTitles > 1 && this.makeFirstVisibleTabClosable) { slottedElements[visibleTabTitlesIndices[0]].closable = true; - this.firstVisibleTabWasClosable = false; + this.makeFirstVisibleTabClosable = false; } this.calciteInternalTabNavSlotChange.emit(slottedElements); @@ -545,7 +545,7 @@ export class TabNav extends LitElement { const totalVisibleTabTitles = visibleTabTitlesIndices.length; if (totalVisibleTabTitles === 1 && tabTitles[visibleTabTitlesIndices[0]].closable) { - this.firstVisibleTabWasClosable = true; + this.makeFirstVisibleTabClosable = true; tabTitles[visibleTabTitlesIndices[0]].closable = false; this.selectedTabId = visibleTabTitlesIndices[0]; From 6cd5028d9a02912971fa76406d31372fa256be31 Mon Sep 17 00:00:00 2001 From: Jose Carcamo Date: Mon, 3 Mar 2025 14:16:51 -0800 Subject: [PATCH 6/6] Addressed code review comments and added test --- .../src/components/tab-nav/tab-nav.tsx | 22 ++++---- .../src/components/tabs/tabs.e2e.ts | 51 +++++++++++++------ 2 files changed, 45 insertions(+), 28 deletions(-) diff --git a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx index 34678e7fcee..da6d6e80e41 100644 --- a/packages/calcite-components/src/components/tab-nav/tab-nav.tsx +++ b/packages/calcite-components/src/components/tab-nav/tab-nav.tsx @@ -12,7 +12,6 @@ import { focusElementInGroup, FocusElementInGroupDestination, getElementDir, - slotChangeGetAssignedElements, } from "../../utils/dom"; import { createObserver } from "../../utils/observers"; import { Scale } from "../interfaces"; @@ -369,24 +368,21 @@ export class TabNav extends LitElement { (event.currentTarget as HTMLDivElement).scrollBy(scrollByX, 0); } - private onSlotChange(event: Event): void { + private onSlotChange(): void { this.intersectionObserver?.disconnect(); - const slottedElements = slotChangeGetAssignedElements( - event, - "calcite-tab-title", - ); - slottedElements.forEach((child) => { + const tabTitles = this.tabTitles; + tabTitles.forEach((child) => { this.intersectionObserver?.observe(child); }); - const visibleTabTitlesIndices = this.visibleTabTitlesIndices; + const visibleTabTitlesIndices = this.getVisibleTabTitlesIndices(tabTitles); const totalVisibleTabTitles = visibleTabTitlesIndices.length; if (totalVisibleTabTitles > 1 && this.makeFirstVisibleTabClosable) { - slottedElements[visibleTabTitlesIndices[0]].closable = true; + tabTitles[visibleTabTitlesIndices[0]].closable = true; this.makeFirstVisibleTabClosable = false; } - this.calciteInternalTabNavSlotChange.emit(slottedElements); + this.calciteInternalTabNavSlotChange.emit(tabTitles); } private storeTabTitleWrapperRef(el: HTMLDivElement) { @@ -529,8 +525,8 @@ export class TabNav extends LitElement { }); } - private get visibleTabTitlesIndices(): number[] { - return this.tabTitles.reduce( + private getVisibleTabTitlesIndices(tabTitles: TabTitle["el"][]): number[] { + return tabTitles.reduce( (tabTitleIndices: number[], tabTitle, index) => !tabTitle.closed ? [...tabTitleIndices, index] : tabTitleIndices, [], @@ -541,7 +537,7 @@ export class TabNav extends LitElement { const { tabTitles } = this; const selectionModified = closedTabTitleEl.selected; - const visibleTabTitlesIndices = this.visibleTabTitlesIndices; + const visibleTabTitlesIndices = this.getVisibleTabTitlesIndices(tabTitles); const totalVisibleTabTitles = visibleTabTitlesIndices.length; if (totalVisibleTabTitles === 1 && tabTitles[visibleTabTitlesIndices[0]].closable) { diff --git a/packages/calcite-components/src/components/tabs/tabs.e2e.ts b/packages/calcite-components/src/components/tabs/tabs.e2e.ts index 2250130cfb3..7155a2a5ef7 100644 --- a/packages/calcite-components/src/components/tabs/tabs.e2e.ts +++ b/packages/calcite-components/src/components/tabs/tabs.e2e.ts @@ -406,23 +406,44 @@ describe("calcite-tabs", () => { expect(selectedTitleOnEmit).toBe("Tab 2 Title"); }); - it("should hide x when last closable tab and display x when > 1 closable tabs", async () => { - for (let i = 2; i <= 4; ++i) { - await page.click(`#tab-title-${i} >>> .${TabTitleCSS.closeButton}`); - } - let tab1 = await page.find(`#tab-title-1`); - expect(await tab1.getProperty("closable")).toBe(false); - expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeNull(); + describe("hiding/displaying X", () => { + it("should hide x when tabs 2 to 4 closed and display x closable tab added", async () => { + for (let i = 2; i <= 4; ++i) { + await page.click(`#tab-title-${i} >>> .${TabTitleCSS.closeButton}`); + } + let tab1 = await page.find(`#tab-title-1`); + expect(await tab1.getProperty("closable")).toBe(false); + expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeNull(); + + await page.evaluate(() => { + document + .getElementById("tab-title-4") + .insertAdjacentHTML("afterend", `Test`); + }); + await page.waitForChanges(); + tab1 = await page.find(`#tab-title-1`); + expect(await tab1.getProperty("closable")).toBe(true); + expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeDefined(); + }); - await page.evaluate(() => { - document - .getElementById("tab-title-4") - .insertAdjacentHTML("afterend", `Test`); + it("should hide x when tabs 1 to 3 closed and display x when closable tab added", async () => { + for (let i = 1; i <= 3; ++i) { + await page.click(`#tab-title-${i} >>> .${TabTitleCSS.closeButton}`); + } + let tab4 = await page.find(`#tab-title-4`); + expect(await tab4.getProperty("closable")).toBe(false); + expect(await page.find(`#tab-title-4 >>> .${TabTitleCSS.closeButton}`)).toBeNull(); + + await page.evaluate(() => { + document + .getElementById("tab-title-4") + .insertAdjacentHTML("afterend", `Test`); + }); + await page.waitForChanges(); + tab4 = await page.find(`#tab-title-4`); + expect(await tab4.getProperty("closable")).toBe(true); + expect(await page.find(`#tab-title-4 >>> .${TabTitleCSS.closeButton}`)).toBeDefined(); }); - await page.waitForChanges(); - tab1 = await page.find(`#tab-title-1`); - expect(await tab1.getProperty("closable")).toBe(true); - expect(await page.find(`#tab-title-1 >>> .${TabTitleCSS.closeButton}`)).toBeDefined(); }); }); });