Skip to content

feat(spinner): add recipe and tokens#31014

Open
thetaPC wants to merge 17 commits intoionic-modularfrom
FW-6860
Open

feat(spinner): add recipe and tokens#31014
thetaPC wants to merge 17 commits intoionic-modularfrom
FW-6860

Conversation

@thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Mar 17, 2026

Issue number: internal


What is the current behavior?

Component styles for ion-spinner are fragmented across multiple files (native and ionic). The md and ios theme uses the native styles. Developers were restricted to those themes and how those themes structured the logic and styles.

What is the new behavior?

  • Unified Style Architecture: Combined theme-specific styling into a single spinner.scss file that consumes CSS variables, ensuring a single source of truth for component logic.
  • Property First model: based the recipe on the properties
  • Updated TypeScript Interface: Changed the filename to spinner.interfaces.ts
  • Defined Theme Defaults: Added defaults that are being used per theme
  • Standardized Sizing: Introduced sizes to md and ios.
  • Updated Test Pages: Updated a couple of test pages to include more cases
  • Updated Tests: Updated tests to include more cases

Does this introduce a breaking change?

  • Yes
  • No

This PR introduces a breaking change to how IonSpinner is styled. Existing manual CSS overrides targeting internal spinner structures or old token names will no longer work due to the shift to the new token hierarchy and a unified base SCSS file.

Migration Path:

  1. Token Updates: Update any custom theme configurations to match the new nested structure.

  2. CSS Overrides: Ensure selectors align with the new slotted element logic and variable names (e.g., --ion-spinner-color).

--color should no longer be used. Setting the value, IonSpinner.color, within the tokens file should be used to change the color.

  1. Theme classes: Remove any instances that target the theme classes: ion-spinner.md

  2. Spinner name class: The class have been updated to include the term name.

.spinner-lines, .spinner-circular, etc should be updated to .spinner-name-lines, .spinner-name-circular, etc.

Other information

Previews:

@vercel
Copy link

vercel bot commented Mar 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ionic-framework Ready Ready Preview, Comment Mar 19, 2026 10:45pm

Request Review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ionic theme for spinner overwrites the medium since it's one of those components that differs to the set value of medium. I decided to not implement it since it's specific to the project. A workaround would be to set it:

ion-spinner {
  --ion-color-medium-bold: #626262;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only removed the word diff from the test file name to line up with the naming conventions of the other components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only removed the word diff from the test file name to line up with the naming conventions of the other components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only removed the word diff from the test file name to line up with the naming conventions of the other components.

@thetaPC thetaPC marked this pull request as ready for review March 18, 2026 20:46
@thetaPC thetaPC requested a review from a team as a code owner March 18, 2026 20:46
@thetaPC thetaPC requested a review from brandyscarney March 18, 2026 20:46
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was renamed, not sure why it's not showing as such.

Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the examples of different spinners with color? I believe those existed to show the color properly applied to all spinner types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see how colors and names needed to be tied together for a test. I would expect that it would work for all names regardless. But I've added them back: 5f0020c

Copy link
Member

Choose a reason for hiding this comment

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

Same question:

Why did you remove the examples of different spinners with color? I believe those existed to show the color properly applied to all spinner types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fn: (dur: number, index: number, total: number) => SpinnerData;
}

interface SpinnerData {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
interface SpinnerData {
export interface SpinnerData {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SpinnerData isn't being used anywhere outside of this file so we don't need to export it.

* Set to `"xlarge"` for the largest size.
*
* Defaults to `"xsmall"` for the `ionic` theme, undefined for all other themes.
* Defaults to `"medium"`.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this only the default for md and ios themes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I chose medium because that one has the values that are currently being used by the community. I also changed the default for ionic to be xsmall through their config.

[spinnerName: string]: SpinnerConfig;
}

export interface SpinnerConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename this? It's kind of confusing when we have IonSpinnerConfig also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on this?: ee53528


circular: {
stroke: {
width: '5.6',
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a token or calculated somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it's exactly the same value set for native.

},

IonSpinner: {
color: '#626262',
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this use a token or are you waiting on the text / gray colors to be added?

iOS has:

color: 'var(--ion-text-color, #000)',

Copy link
Contributor Author

@thetaPC thetaPC Mar 19, 2026

Choose a reason for hiding this comment

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

No, this is due to ionic currently using their own tokens. Since we are no longer using them, I just grabbed the raw value that represent that token. They can always create their own theme and use their tokens.


lines: {
stroke: {
width: '7px',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add variables for this, or should we use border-width ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

border-width doesn't have a 7px value. Even if it did, I still would expect to use scaling since the stroke width targets a svg not a border.

I don't think we should add a variable to account for 7px because it would break the current implementation we have for tokens like scaling. We are incrementing the keys by 50 so odd values aren't supported.


lines: {
stroke: {
width: '7px',
Copy link
Member

Choose a reason for hiding this comment

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

Same question as ios

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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