- 
                Notifications
    
You must be signed in to change notification settings  - Fork 13.4k
 
feat(button): add spinner sizes for ionic theme #30233
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
Conversation
| 
           The latest updates on your projects. Learn more about Vercel for Git ↗︎ 
  | 
    
55127c5    to
    0d88065      
    Compare
  
    also update e2e tests spanshots
af6c299    to
    0865a33      
    Compare
  
    | import { expect } from '@playwright/test'; | ||
| import { configs, test } from '@utils/test/playwright'; | ||
| 
               | 
          ||
| configs({ modes: ['ionic-md', 'ionic-ios'] }).forEach(({ config, screenshot, title }) => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the functionality doesn't change, we don't need to test for all the modes. I also realize that we don't need to test RTL since functionality doesn't change.
I suggest only testing ionic-md. Make sure to remove the snapshots for ionic-ios and RTL.
| configs({ modes: ['ionic-md', 'ionic-ios'] }).forEach(({ config, screenshot, title }) => { | |
| configs({ modes: ['ionic-md'], directions: ['ltr'] }).forEach(({ config, screenshot, title }) => { | 
f816a29    to
    f5ee753      
    Compare
  
    691475f    to
    8ba6fb2      
    Compare
  
    Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
8ba6fb2    to
    18312f7      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RTL snapshots need to be removed since those aren't being tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue number: internal
What is the current behavior?
Spinners inside buttons don't have any specific styling.
What is the new behavior?
According to the design, for the ionic theme, the size of spinners inside buttons need specific values. To accommodate those:
Does this introduce a breaking change?