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.

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

feat(tab-button): add the shape property and styles for the ionic theme #30057

Merged
merged 14 commits into from
Dec 6, 2024
1 change: 1 addition & 0 deletions core/api.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2213,6 +2213,7 @@ ion-tab-button,prop,layout,"icon-bottom" | "icon-end" | "icon-hide" | "icon-star
ion-tab-button,prop,mode,"ios" | "md",undefined,false,false
ion-tab-button,prop,rel,string | undefined,undefined,false,false
ion-tab-button,prop,selected,boolean,false,false,false
ion-tab-button,prop,shape,"rectangular" | "round" | "soft" | undefined,undefined,false,false
ion-tab-button,prop,tab,string | undefined,undefined,false,false
ion-tab-button,prop,target,string | undefined,undefined,false,false
ion-tab-button,prop,theme,"ios" | "md" | "ionic",undefined,false,false
Expand Down
8 changes: 8 additions & 0 deletions core/src/components.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3504,6 +3504,10 @@ export namespace Components {
* The selected tab component
*/
"selected": boolean;
/**
* Set to `"soft"` for a tab-button with slightly rounded corners, `"round"` for a tab-button with fully rounded corners, or `"rectangular"` for a tab-button without rounded corners. Defaults to `"round"` for the `ionic` theme, undefined for all other themes.
*/
"shape"?: 'soft' | 'round' | 'rectangular';
/**
* A tab id must be provided for each `ion-tab`. It's used internally to reference the selected tab or by the router to switch between them.
*/
Expand Down Expand Up @@ -8941,6 +8945,10 @@ declare namespace LocalJSX {
* The selected tab component
*/
"selected"?: boolean;
/**
* Set to `"soft"` for a tab-button with slightly rounded corners, `"round"` for a tab-button with fully rounded corners, or `"rectangular"` for a tab-button without rounded corners. Defaults to `"round"` for the `ionic` theme, undefined for all other themes.
*/
"shape"?: 'soft' | 'round' | 'rectangular';
/**
* A tab id must be provided for each `ion-tab`. It's used internally to reference the selected tab or by the router to switch between them.
*/
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 15 additions & 2 deletions core/src/components/tab-button/tab-button.ionic.scss
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
--focus-ring-color: #{globals.$ion-border-focus-default};
--focus-ring-width: #{globals.$ion-border-radius-025};

@include globals.border-radius(globals.$ion-border-radius-200);

align-content: center;

min-height: globals.$ion-scale-1200;
Expand Down Expand Up @@ -73,3 +71,18 @@
width: globals.$ion-scale-600;
height: globals.$ion-scale-600;
}

// Tab Button Shapes
// -------------------------------------------------------------------------------

:host(.tab-button-shape-soft) {
@include globals.border-radius(globals.$ion-border-radius-200);
}

:host(.tab-button-shape-round) {
@include globals.border-radius(globals.$ion-border-radius-400);
}

:host(.tab-button-shape-rectangular) {
@include globals.border-radius(globals.$ion-border-radius-0);
}
27 changes: 27 additions & 0 deletions core/src/components/tab-button/tab-button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ export class TabButton implements ComponentInterface, AnchorInterface {
*/
@Prop({ mutable: true }) selected = false;

/**
* Set to `"soft"` for a tab-button with slightly rounded corners,
* `"round"` for a tab-button with fully rounded corners, or `"rectangular"`
* for a tab-button without rounded corners.
*
* Defaults to `"round"` for the `ionic` theme, undefined for all other themes.
*/
@Prop() shape?: 'soft' | 'round' | 'rectangular';

/**
* A tab id must be provided for each `ion-tab`. It's used internally to reference
* the selected tab or by the router to switch between them.
Expand Down Expand Up @@ -107,6 +116,22 @@ export class TabButton implements ComponentInterface, AnchorInterface {
}
}

private getShape(): string | undefined {
const theme = getIonTheme(this);
const { shape } = this;

// TODO(ROU-11436): Remove theme check when shapes are defined for all themes.
if (theme !== 'ionic') {
return undefined;
}

if (shape === undefined) {
return 'round';
}

return shape;
}

private selectTab(ev: Event | KeyboardEvent) {
if (this.tab !== undefined) {
if (!this.disabled) {
Expand Down Expand Up @@ -141,6 +166,7 @@ export class TabButton implements ComponentInterface, AnchorInterface {
render() {
const { disabled, hasIcon, hasLabel, href, rel, target, layout, selected, tab, inheritedAttributes } = this;
const theme = getIonTheme(this);
const shape = this.getShape();
const attrs = {
download: this.download,
href,
Expand All @@ -164,6 +190,7 @@ export class TabButton implements ComponentInterface, AnchorInterface {
[`tab-layout-${layout}`]: true,
'ion-activatable': true,
'ion-selectable': true,
[`tab-button-shape-${shape}`]: shape !== undefined,
'ion-focusable': true,
}}
>
Expand Down
55 changes: 55 additions & 0 deletions core/src/components/tab-button/test/shape/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Tab Button - Shape</title>
<meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0" />
<link rel="stylesheet" href="../../../../../css/ionic.bundle.css" />
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" />
<script src="../../../../../scripts/testing/scripts.js"></script>
<script nomodule src="../../../../../dist/ionic/ionic.js"></script>
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
</head>

<body>
<ion-app>
<ion-header>
<ion-toolbar>
<ion-title>Tab Button - Shape</ion-title>
</ion-toolbar>
</ion-header>

<ion-content>
<ion-text> (Only available for ionic theme) </ion-text>

<ion-tab-bar>
<ion-tab-button>
<ion-label>Default Shape</ion-label>
<ion-icon aria-hidden="true" name="triangle-outline"></ion-icon>
</ion-tab-button>

<ion-tab-button id="soft-tab-button" shape="soft">
<ion-label>Soft Shape</ion-label>
<ion-icon aria-hidden="true" name="triangle-outline"></ion-icon>
</ion-tab-button>

<ion-tab-button id="round-tab-button" shape="round">
<ion-label>Round Shape</ion-label>
<ion-icon aria-hidden="true" name="triangle-outline"></ion-icon>
</ion-tab-button>

<ion-tab-button id="rect-tab-button" shape="rectangular">
<ion-label>Rectangular Shape</ion-label>
<ion-icon aria-hidden="true" name="triangle-outline"></ion-icon>
</ion-tab-button>
</ion-tab-bar>
</ion-content>

<style>
ion-tab-bar {
background: #d4d4d4;
}
</style>
</ion-app>
</body>
</html>
51 changes: 51 additions & 0 deletions core/src/components/tab-button/test/shape/tab-button.e2e.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { expect } from '@playwright/test';
import { configs, test } from '@utils/test/playwright';
import type { E2EPage, E2EPageOptions, ScreenshotFn } from '@utils/test/playwright';

class TabButtonFixture {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was a fixture created?

Copy link
Author

Choose a reason for hiding this comment

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

This file was created based on toast/test/shape/toast.e2e.ts which also has a fixture and it made sense for me to have one here in order to access the same page and elements on every test.

readonly page: E2EPage;

constructor(page: E2EPage) {
this.page = page;
}

async goto(config: E2EPageOptions) {
const { page } = this;
await page.goto(`/src/components/tab-button/test/shape`, config);
}

async screenshot(screenshotModifier: string, screenshot: ScreenshotFn, buttonId: string) {
const { page } = this;

const screenshotString = screenshot(`tab-button-${screenshotModifier}`);
const tabButton = page.locator(buttonId);

await expect(tabButton).toHaveScreenshot(screenshotString);
}
}

/**
* This behavior does not vary across directions.
*/
configs({ modes: ['ionic-md'], directions: ['ltr'] }).forEach(({ config, screenshot, title }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

All the screenshots are wrong. They do not match the HTML test page. Please fix.

Copy link
Author

Choose a reason for hiding this comment

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

I have since fixed those screenshots. However, the basic tab-bar tests were also failing. On the one hand, it makes sense since the default button corners have changed. On the other hand, I do not understand how the border color could have changed:
image
Perhaps it's related to design tokens?

Copy link
Author

@OS-pedrolourenco OS-pedrolourenco Dec 4, 2024

Choose a reason for hiding this comment

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

I also just noticed that the snapshots for the "darwin" OS for the tab-button are wrong but somehow the tests pass locally and on the PR checks 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

darwin snapshots are from tests that were run on your local computer. Those snapshots may vary based on the computer, for example snapshots on a Windows device will most likely not be the same as a Mac device. So they're not considered the true source and are not allowed to be pushed to the repro.

In order to make sure snapshots are always being generated correctly to the same environment as the CI, we run the tests within Docker: npm run test.e2e.docker. You'll know that the snapshots are correct when they have the linux keyword within the file.

Make sure you are using that command to run the tests. You can also use npm run test.e2e.docker.update-snapshots to update the true snapshots locally.

In order to fix this PR, I suggest running:

  1. npm run test.e2e.docker tab-bar tab-button. This command will run all the tests that have the keywords tab-bar or tab-bar.
  2. Review the failing tests: npx playwright show-report
  3. It's extremely important that you understand why the tests are failing. They should only be failing for ionic theme and anything related to the tab button radius.
  4. If you feel confident, then update the snapshots: npm run test.e2e.docker.update-snapshots tab-button tab-bar. You might not need to update both components so adjust the command as needed.

Copy link
Author

Choose a reason for hiding this comment

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

I see! That makes a more sense! Thanks for the context!
I noticed yesterday or the day before that I was running snapshot tests incorrectly and started using Docker like you mentioned. What I wasn't aware was that those "darwin" files were leftovers from running the tests incorrectly. I will remove them then.

By the way, did you get a chance to look at my first comment regarding the unexpected change in border color for the tab-button?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did, that's why I said to make sure you review the failing tests. I'm not seeing a change of color. The tests are failing for another reason.

Copy link
Author

Choose a reason for hiding this comment

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

As I said before, the remaining tab-bar tests are failing due to the default border corners now being different. But, besides that, I also find it odd that the border color for the tab-button in the image I sent here has also changed to a different shade of blue. As such, I am raising a flag before updating the snapshots that might be incorrect.

Copy link
Author

@OS-pedrolourenco OS-pedrolourenco Dec 5, 2024

Choose a reason for hiding this comment

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

After looking into it a bit more I can indeed confirm that the color change was caused by the change in design tokens here. Thus, I conclude that, in general, snapshot tests do not check for color changes since these tests did not break with Bernardo's PR. I will then proceed with updating the snapshots.

test.describe(title('tab-button: shape'), () => {
let tabButtonFixture: TabButtonFixture;

test.beforeEach(async ({ page }) => {
tabButtonFixture = new TabButtonFixture(page);
await tabButtonFixture.goto(config);
});

test('should render a soft tab button', async () => {
await tabButtonFixture.screenshot('shape-soft', screenshot, '#soft-tab-button');
});

test('should render a round tab button', async () => {
await tabButtonFixture.screenshot('shape-round', screenshot, '#round-tab-button');
});

test('should render a rectangular tab button', async () => {
await tabButtonFixture.screenshot('shape-rectangular', screenshot, '#rect-tab-button');
});
});
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions packages/angular/src/directives/proxies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2276,14 +2276,14 @@ export declare interface IonTabBar extends Components.IonTabBar {}


@ProxyCmp({
inputs: ['disabled', 'download', 'href', 'layout', 'mode', 'rel', 'selected', 'tab', 'target', 'theme']
inputs: ['disabled', 'download', 'href', 'layout', 'mode', 'rel', 'selected', 'shape', 'tab', 'target', 'theme']
})
@Component({
selector: 'ion-tab-button',
changeDetection: ChangeDetectionStrategy.OnPush,
template: '<ng-content></ng-content>',
// eslint-disable-next-line @angular-eslint/no-inputs-metadata-property
inputs: ['disabled', 'download', 'href', 'layout', 'mode', 'rel', 'selected', 'tab', 'target', 'theme'],
inputs: ['disabled', 'download', 'href', 'layout', 'mode', 'rel', 'selected', 'shape', 'tab', 'target', 'theme'],
})
export class IonTabButton {
protected el: HTMLElement;
Expand Down
4 changes: 2 additions & 2 deletions packages/angular/standalone/src/directives/proxies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2081,14 +2081,14 @@ export declare interface IonTabBar extends Components.IonTabBar {}

@ProxyCmp({
defineCustomElementFn: defineIonTabButton,
inputs: ['disabled', 'download', 'href', 'layout', 'mode', 'rel', 'selected', 'tab', 'target', 'theme']
inputs: ['disabled', 'download', 'href', 'layout', 'mode', 'rel', 'selected', 'shape', 'tab', 'target', 'theme']
})
@Component({
selector: 'ion-tab-button',
changeDetection: ChangeDetectionStrategy.OnPush,
template: '<ng-content></ng-content>',
// eslint-disable-next-line @angular-eslint/no-inputs-metadata-property
inputs: ['disabled', 'download', 'href', 'layout', 'mode', 'rel', 'selected', 'tab', 'target', 'theme'],
inputs: ['disabled', 'download', 'href', 'layout', 'mode', 'rel', 'selected', 'shape', 'tab', 'target', 'theme'],
standalone: true
})
export class IonTabButton {
Expand Down
Loading