Skip to content

fix(spinner): allow resizing of dots, bubbles, and circles #27424

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

Merged
merged 14 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
10 changes: 5 additions & 5 deletions core/src/components/spinner/spinner-configs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ const spinners = {
return {
r: 5,
style: {
top: `${9 * Math.sin(angle)}px`,
left: `${9 * Math.cos(angle)}px`,
top: `${32 * Math.sin(angle)}%`,
left: `${32 * Math.cos(angle)}%`,
'animation-delay': animationDelay,
},
};
Expand All @@ -28,8 +28,8 @@ const spinners = {
return {
r: 5,
style: {
top: `${9 * Math.sin(angle)}px`,
left: `${9 * Math.cos(angle)}px`,
top: `${32 * Math.sin(angle)}%`,
left: `${32 * Math.cos(angle)}%`,
'animation-delay': animationDelay,
},
};
Expand Down Expand Up @@ -72,7 +72,7 @@ const spinners = {
return {
r: 6,
style: {
left: `${9 - 9 * index}px`,
left: `${32 - 32 * index}%`,
'animation-delay': animationDelay,
},
};
Expand Down
117 changes: 117 additions & 0 deletions core/src/components/spinner/test/resize/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Spinner - Resize</title>
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no"
/>
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" />
<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>
<style>
ion-spinner {
width: 100px;
height: 100px;
}
</style>
</head>

<body>
<ion-app>
<ion-header>
<ion-toolbar>
<ion-title>Spinner - Resize</ion-title>
</ion-toolbar>
</ion-header>

<ion-content id="content">
<ion-list>
<ion-list-header>
<ion-label>Spinner Loading Indicators</ion-label>
</ion-list-header>
<ion-item><ion-spinner slot="start"></ion-spinner>Platform Default Spinner</ion-item>
<ion-item>
<ion-spinner slot="start" name="lines"></ion-spinner>
<code>lines</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="lines-small"></ion-spinner>
<code>lines-small</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="lines-sharp"></ion-spinner>
<code>lines-sharp</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="lines-sharp-small"></ion-spinner>
<code>lines-sharp-small</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="circular"></ion-spinner>
<code>circular</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="dots"></ion-spinner>
<code>dots</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="bubbles"></ion-spinner>
<code>bubbles</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="circles"></ion-spinner>
<code>circles</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="crescent"></ion-spinner>
<code>crescent</code>
</ion-item>
</ion-list>

<ion-list>
<ion-item><ion-spinner slot="start" paused></ion-spinner>Paused Platform Default Spinner</ion-item>
<ion-item>
<ion-spinner slot="start" name="lines" paused></ion-spinner>
<code>lines</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="lines-small" paused></ion-spinner>
<code>lines-small</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="lines-sharp" paused></ion-spinner>
<code>lines-sharp</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="lines-sharp-small" paused></ion-spinner>
<code>lines-sharp-small</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="circular" paused></ion-spinner>
<code>circular</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="dots" paused></ion-spinner>
<code>dots</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="bubbles" paused></ion-spinner>
<code>bubbles</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="circles" paused></ion-spinner>
<code>circles</code>
</ion-item>
<ion-item>
<ion-spinner slot="start" name="crescent" paused></ion-spinner>
<code>crescent</code>
</ion-item>
</ion-list>
</ion-content>
</ion-app>
</body>
</html>
20 changes: 20 additions & 0 deletions core/src/components/spinner/test/resize/spinner.e2e.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { expect } from '@playwright/test';
import { configs, test } from '@utils/test/playwright';

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

Choose a reason for hiding this comment

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

Spinner has no per-mode styles, so we don't need both iOS and MD screenshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Should I change the other spinner tests similarly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah you can, though spinner does have per-mode behavior, so we should probably have a spec test that checks that the default spinner is set correctly on each mode:

private getName(): SpinnerTypes {
const spinnerName = this.name || config.get('spinner');
const mode = getIonMode(this);
if (spinnerName) {
return spinnerName;
}
return mode === 'ios' ? 'lines' : 'circular';

We might be able to check that the correct spinner-* class is set.

I'd also be fine if we made a ticket and updated the non-resize tests as a separate task.

test.describe(title('spinner: resize'), () => {
test.beforeEach(async ({ page }) => {
await page.goto('/src/components/spinner/test/resize', config);
});
test.describe('spinner: visual regression tests', () => {
test('should not have visual regressions', async ({ page }) => {
await page.setIonViewport();

await expect(page).toHaveScreenshot(screenshot(`spinner-resize-diff`));
Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a lot of wasted space in the screenshot which will slow things down on CI. Could we instead have each spinner next to each other so we can take a smaller screenshot?

});
});
});
});
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.