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

Conversation

mapsandapps
Copy link
Contributor

@mapsandapps mapsandapps commented May 8, 2023

Issue number: Resolves #18115


What is the current behavior?

Resizing the ion-spinner by setting the width & height using CSS works for most spinner variants, but not for dots, bubbles, and circles. The spacing between the circles on these spinner variants is not scaling with the spinner.

What is the new behavior?

  • ion-spinner may now be resized by setting the width & height using CSS for all variants.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Before

Screenshot 2023-05-08 at 5 17 59 PM

After

Screenshot 2023-05-08 at 5 18 45 PM

@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label May 8, 2023
@mapsandapps mapsandapps marked this pull request as ready for review May 8, 2023 22:23
@mapsandapps mapsandapps requested a review from a team as a code owner May 8, 2023 22:23
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

This looks fantastic, great work!

I updated the PR description to include Resolve # so that the ticket is automatically closed when the PR is merged.

/**
* 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('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?

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Some questions/nitpicks, but overall this looks great. Nice work! Good to merge when my comments below have been addressed.

config
);

await expect(page.locator('#content')).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.

If you choose to remove ion-content (see other comment), then you can just do page.

Suggested change
await expect(page.locator('#content')).toHaveScreenshot(screenshot(`spinner-resize-diff`));
await expect(page).toHaveScreenshot(screenshot(`spinner-resize-diff`));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the tips!

@mapsandapps mapsandapps added this pull request to the merge queue May 12, 2023
Merged via the queue into main with commit e5ae45d May 12, 2023
@mapsandapps mapsandapps deleted the FW-3692 branch May 12, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: increasing size of ion-spinner results in blurry animation on iOS
4 participants