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

Open
wants to merge 6 commits into
base: next
Choose a base branch
from

Conversation

OS-pedrolourenco
Copy link

Issue number: internal


What is the current behavior?

Tab button does not have a shape property.

What is the new behavior?

  • Adds support for the shape property in tab button.
  • Adds styles for the "soft", "round" and "rectangular" shapes in the ionic theme
  • Defaults the shape to "round" for the ionic theme
  • Adds an e2e test for shape with screenshots of all shapes

Does this introduce a breaking change?

  • Yes
  • No

Other information

Copy link

vercel bot commented Dec 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2024 9:57am

Comment on lines +120 to +123
const { shape } = this;

// TODO(ROU-11300): Remove theme check when shapes are defined for all themes.
if (getIonTheme(this) !== 'ionic') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this consistent with the other getShape functions.

Suggested change
const { shape } = this;
// TODO(ROU-11300): Remove theme check when shapes are defined for all themes.
if (getIonTheme(this) !== 'ionic') {
const theme = getIonTheme(this);
const { shape } = this;
// TODO(ROU-11300): Remove theme check when shapes are defined for all themes.
if (theme !== 'ionic') {

@@ -164,6 +189,7 @@ export class TabButton implements ComponentInterface, AnchorInterface {
[`tab-layout-${layout}`]: true,
'ion-activatable': true,
'ion-selectable': true,
[`toast-shape-${shape}`]: shape !== undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[`toast-shape-${shape}`]: shape !== undefined,
[`tab-shape-${shape}`]: shape !== undefined,

@@ -73,3 +71,18 @@
width: globals.$ion-scale-600;
height: globals.$ion-scale-600;
}

// Toast Shapes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Toast Shapes
// Tab Button Shapes

// Toast Shapes
// -------------------------------------------------------------------------------

:host(.toast-shape-soft) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:host(.toast-shape-soft) {
:host(.tab-shape-soft) {

border-radius: #{globals.$ion-border-radius-200};
}

:host(.toast-shape-round) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:host(.toast-shape-round) {
:host(.tab-shape-round) {


<ion-tab-bar>
<ion-tab-button id="default-tab-button" style="background: #ff9e99">
<ion-icon aria-hidden="true" name="stop-circle"></ion-icon>
Copy link
Contributor

Choose a reason for hiding this comment

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

Use triangle icons for all the tab buttons. This matches Figma designs and it makes it easier to debug when we stick as close to Figma as possible.

Suggested change
<ion-icon aria-hidden="true" name="stop-circle"></ion-icon>
<ion-icon aria-hidden="true" name="triangle-outline"></ion-icon>

</ion-toolbar>
</ion-header>

<ion-content id="content" style="text-align: center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we want to render a simple and pure version of tab button, remove the style.

Suggested change
<ion-content id="content" style="text-align: center">
<ion-content id="content">

</ion-toolbar>
</ion-header>

<ion-content id="content" style="text-align: center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that an id attribute was added? If not, then remove.

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.

Comment on lines +39 to +41
test('should render the default tab button', async () => {
await tabButtonFixture.screenshot('shape-round', screenshot, '#default-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.

Remove, defaults are needed to be tested since those are already tested in a default tab bar test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants