-
Notifications
You must be signed in to change notification settings - Fork 13.4k
feat(tabs): migrate to Modular Ionic #31116
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.
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
Changes from all commits
d8cd250
51f637f
29a58aa
54c7161
61b64bb
8c08957
5d57b05
5436bfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| @use "../../themes/common.globals" as globals; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this not changed to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| @import "../../themes/native/native.globals"; | ||
|
|
||
| :host { | ||
|
|
@@ -6,5 +7,5 @@ | |
| position: absolute; | ||
|
|
||
| contain: layout size style; | ||
| z-index: $z-index-page-container; | ||
| z-index: globals.$z-index-page-container; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was adding the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we've been doing that with all the components we've migrated. Should we be using singular? If so, I can make those changes in a different PR. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,6 @@ import type { TabButtonClickEventDetail } from '../tab-bar/tab-bar-interface'; | |
|
|
||
| /** | ||
| * @virtualProp {"ios" | "md"} mode - The mode determines the platform behaviors of the component. | ||
| * @virtualProp {"ios" | "md" | "ionic"} theme - The theme determines the visual appearance of the component. | ||
| * | ||
| * @slot - Content is placed between the named slots if provided without a slot. | ||
| * @slot top - Content is placed at the top of the screen. | ||
|
|
@@ -82,7 +81,7 @@ export class Tabs implements NavOutlet { | |
| return; | ||
| } | ||
|
|
||
| const tab = this.selectedTab ? this.selectedTab.tab : undefined; | ||
| const tab = this.selectedTab?.tab; | ||
|
|
||
| // If tabs has no selected tab but tab-bar already has a selected-tab set, | ||
| // don't overwrite it. This handles cases where tab-bar is used without ion-tab elements. | ||
|
|
@@ -130,7 +129,7 @@ export class Tabs implements NavOutlet { | |
| */ | ||
| @Method() | ||
| getSelected(): Promise<string | undefined> { | ||
| return Promise.resolve(this.selectedTab ? this.selectedTab.tab : undefined); | ||
| return Promise.resolve(this.selectedTab?.tab); | ||
| } | ||
|
|
||
| /** @internal */ | ||
|
|
@@ -158,7 +157,7 @@ export class Tabs implements NavOutlet { | |
|
|
||
| private setActive(selectedTab: HTMLIonTabElement): Promise<void> { | ||
| if (this.transitioning) { | ||
| return Promise.reject('transitioning already happening'); | ||
| return Promise.reject(new Error('transitioning already happening')); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change attaches a stack trace to the rejection, making it easier to identify where the error originated when debugging. |
||
| } | ||
|
|
||
| this.transitioning = true; | ||
|
|
@@ -190,10 +189,7 @@ export class Tabs implements NavOutlet { | |
|
|
||
| private notifyRouter() { | ||
| if (this.useRouter) { | ||
| const router = document.querySelector('ion-router'); | ||
| if (router) { | ||
| return router.navChanged('forward'); | ||
| } | ||
| return this.router?.navChanged('forward'); | ||
| } | ||
| return Promise.resolve(false); | ||
| } | ||
|
|
@@ -207,13 +203,14 @@ export class Tabs implements NavOutlet { | |
| return Array.from(this.el.querySelectorAll('ion-tab')); | ||
| } | ||
|
|
||
| private get router() { | ||
| return document.querySelector('ion-router'); | ||
| } | ||
|
|
||
| private onTabClicked = (ev: CustomEvent<TabButtonClickEventDetail>) => { | ||
| const { href, tab } = ev.detail; | ||
| if (this.useRouter && href !== undefined) { | ||
| const router = document.querySelector('ion-router'); | ||
| if (router) { | ||
| router.push(href); | ||
| } | ||
| this.router?.push(href); | ||
| } else { | ||
| this.select(tab); | ||
| } | ||
|
|
@@ -236,7 +233,7 @@ const getTab = (tabs: HTMLIonTabElement[], tab: string | HTMLIonTabElement): HTM | |
| const tabEl = typeof tab === 'string' ? tabs.find((t) => t.tab === tab) : tab; | ||
|
|
||
| if (!tabEl) { | ||
| printIonError(`[ion-tabs] - Tab with id: "${tabEl}" does not exist`); | ||
| printIonError(`[ion-tabs] - Tab with id: "${tab}" does not exist`); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This kept showing as |
||
| } | ||
| return tabEl; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| <html lang="en" dir="ltr"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <title>Tab - Basic</title> | ||
| <title>Tabs - Basic</title> | ||
| <meta | ||
| name="viewport" | ||
| content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no, viewport-fit=cover" | ||
|
|
@@ -42,8 +42,6 @@ | |
| </ion-header> | ||
| <ion-content class="ion-padding"> | ||
| <h1>Tab One</h1> | ||
| <button class="expand" onclick="updateBadgeCount()">Update Badge Count</button> | ||
| <button class="expand" onclick="updateBadgeColor()">Update Badge Color</button> | ||
|
Comment on lines
-45
to
-46
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These buttons weren't doing anything. I do plan on adding |
||
|
|
||
| <ion-fab slot="fixed" horizontal="end" vertical="bottom"> | ||
| <ion-fab-button class="custom-white"> | ||
|
|
@@ -125,11 +123,11 @@ <h1>Hidden Tab</h1> | |
| this.innerHTML = ` | ||
| <ion-header> | ||
| <ion-toolbar> | ||
| <ion-title>Page Four</ion-title> | ||
| <ion-title>Tab Four</ion-title> | ||
| </ion-toolbar> | ||
| </ion-header> | ||
| <ion-content class="ion-padding"> | ||
| Page Four | ||
| <h1>Tab Four</h1> | ||
| </ion-content> | ||
| `; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| import { newSpecPage } from '@stencil/core/testing'; | ||
|
|
||
| import { Tab } from '../../tab/tab'; | ||
| import { Tabs } from '../tabs'; | ||
|
|
||
| const HTML = ` | ||
| <ion-tabs> | ||
| <ion-tab tab="tab-one"></ion-tab> | ||
| <ion-tab tab="tab-two"></ion-tab> | ||
| </ion-tabs> | ||
| `; | ||
|
|
||
| describe('ion-tabs', () => { | ||
| describe('getSelected()', () => { | ||
| it('should return the name of the initially selected tab', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
| expect(await tabsEl.getSelected()).toBe('tab-one'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getTab()', () => { | ||
| let consoleErrorSpy: jest.SpyInstance; | ||
|
|
||
| beforeEach(() => { | ||
| // getTab() calls printIonError when the tab id is not found, suppress to keep test output clean | ||
| consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| consoleErrorSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('should return the element for an existing tab id', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
| const tabEl = page.body.querySelector('ion-tab[tab="tab-two"]')!; | ||
| expect(await tabsEl.getTab('tab-two')).toBe(tabEl); | ||
| }); | ||
|
|
||
| it('should return undefined for a non-existent tab id', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
| expect(await tabsEl.getTab('does-not-exist')).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('select()', () => { | ||
| let consoleErrorSpy: jest.SpyInstance; | ||
|
|
||
| beforeEach(() => { | ||
| // select() calls printIonError when the tab id is not found, suppress to keep test output clean | ||
| consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| consoleErrorSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('should switch to the specified tab and return true', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
| const result = await tabsEl.select('tab-two'); | ||
|
|
||
| expect(result).toBe(true); | ||
| expect(await tabsEl.getSelected()).toBe('tab-two'); | ||
| }); | ||
|
|
||
| it('should return false when selecting the already active tab', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
| const result = await tabsEl.select('tab-one'); | ||
|
|
||
| expect(result).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false when selecting a non-existent tab id', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
| const result = await tabsEl.select('does-not-exist'); | ||
|
|
||
| expect(result).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('events', () => { | ||
| it('should emit ionTabsWillChange and ionTabsDidChange when switching tabs', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
|
|
||
| const willChangeSpy = jest.fn(); | ||
| const didChangeSpy = jest.fn(); | ||
| tabsEl.addEventListener('ionTabsWillChange', willChangeSpy); | ||
| tabsEl.addEventListener('ionTabsDidChange', didChangeSpy); | ||
|
|
||
| await tabsEl.select('tab-two'); | ||
| await page.waitForChanges(); | ||
|
|
||
| expect(willChangeSpy).toHaveBeenCalledWith(expect.objectContaining({ detail: { tab: 'tab-two' } })); | ||
| expect(didChangeSpy).toHaveBeenCalledWith(expect.objectContaining({ detail: { tab: 'tab-two' } })); | ||
| }); | ||
|
|
||
| it('should not emit ionTabsDidChange when selecting the already active tab', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
|
|
||
| const didChangeSpy = jest.fn(); | ||
| tabsEl.addEventListener('ionTabsDidChange', didChangeSpy); | ||
|
|
||
| await tabsEl.select('tab-one'); | ||
|
|
||
| expect(didChangeSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('error handling', () => { | ||
| let consoleErrorSpy: jest.SpyInstance; | ||
|
|
||
| beforeEach(() => { | ||
| // select() calls printIonError when the tab id is not found, suppress to keep test output clean | ||
| consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| consoleErrorSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('should log the correct tab id when selecting a non-existent tab', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Tabs, Tab], | ||
| html: HTML, | ||
| }); | ||
|
|
||
| const tabsEl = page.body.querySelector('ion-tabs')!; | ||
| await tabsEl.select('does-not-exist'); | ||
|
|
||
| expect(consoleErrorSpy).toHaveBeenCalledWith( | ||
| '[Ionic Error]: [ion-tabs] - Tab with id: "does-not-exist" does not exist' | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this not changed to
@use?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to minimize the changes in other components as much as possible. The
usemigration will be done when we migratenavto ionic modular. This is just a precaution in case an issue occurs. I don't want to spend more time on components that are not in the scope of tickets.