Skip to content

fix(loading): spinner now respects —spinner-color#25261

Merged
liamdebeasi merged 12 commits intomainfrom
FW-1395
May 12, 2022
Merged

fix(loading): spinner now respects —spinner-color#25261
liamdebeasi merged 12 commits intomainfrom
FW-1395

Conversation

@liamdebeasi
Copy link
Copy Markdown
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue URL: resolves #25180

What is the new behavior?

  • All spinners now have the correct color set when using inside of ion-loading

Does this introduce a breaking change?

  • Yes
  • No

Other information

Note: Rather than add another test for this specific usage, I updated the existing test to account for this new behavior.

@liamdebeasi liamdebeasi requested a review from a team May 9, 2022 18:37
@github-actions github-actions bot added the package: core @ionic/core package label May 9, 2022
Copy link
Copy Markdown
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

Would it be possible to add a timeout or something so the loading spinner is more visible in the screenshots? I worry that if this does regress in the future, it'll be hard to tell what's going on. 🤔

Also, one of the popover screenshots got updated and the popover is now missing; looks like that test might be flaky?

@liamdebeasi
Copy link
Copy Markdown
Contributor Author

liamdebeasi commented May 10, 2022

That's the initial state of the spinner -- all of the spinners are paused by default so they won't be running. Adding a timeout won't change how the spinner is rendered as a result. I can try changing it to another spinner that has more visible in the initial state.

@liamdebeasi
Copy link
Copy Markdown
Contributor Author

re: popover screenshots: that looks like FW-1397. Let me take a look and see if I can fix it.

@liamdebeasi
Copy link
Copy Markdown
Contributor Author

Ok I did a couple things:

  1. Opened test(picker-internal): skip overlay test on webkit #25267 which should correctly skip the picker internal test on WebKit. Will sync those changes into this branch when I merge the PR.
  2. Updated the loading test to use lines-sharp. This is another one of the affected spinners but has more content rendered in the initial paused state.

@liamdebeasi liamdebeasi requested a review from averyjohnston May 10, 2022 15:19
@averyjohnston
Copy link
Copy Markdown
Contributor

Much better, thanks!

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: circular, lines-sharp, and lines-sharp-small not getting --spinner-color applied in ion-loading

3 participants