- Notifications
You must be signed in to change notification settings - Fork13.4k
feat(chip): add recipe and variables#30873
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:ionic-modular
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vercelbot commentedDec 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The latest updates on your projects. Learn more aboutVercel for GitHub.
|
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.
Do we need Sass variables for this? I feel like it just adds another layer when the CSS variables would do
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.
I created it so we don't have such long variables cluttering the base file. Thoughts?
Uh oh!
There was an error while loading.Please reload this page.
| }; | ||
| typeComponents={ | ||
| IonChip?:{ |
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.
Do you think we should keep these interfaces in the component's folder to keep it better organized? Or is this something we might move later and combine between components?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
core/src/themes/md/default.tokens.ts Outdated
| paddingVertical:'6px', | ||
| paddingHorizontal:'12px', |
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.
| paddingVertical:'6px', | |
| paddingHorizontal:'12px', | |
| padding:{ | |
| vertical:'6px', | |
| horizontal:'12px', | |
| } |
Would it not be cleaner to do it as objects?
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.
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.
Looks good. We might want to move totop,end,start andbottom for all components for consistency but we can tackle that when we get there.
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.
A lot of these values are hardcoded - why aren't we using the design tokens like this:
--ion-scaling-xxs--ion-scaling-xs--ion-scaling-smetc| }, | ||
| avatar:{ | ||
| size:`${24/fontSizes.chipBase}em`, |
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.
These numbers seem made up - maybe we need to define these calculations better?
| } | ||
| constfontSizes={ | ||
| chipBase:14, |
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.
Why is the base 14 for Chip? Shouldn't all components have the same base size?
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.
My comments on the md file apply here as well
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Issue number: resolves #
What is the current behavior?
What is the new behavior?
Does this introduce a breaking change?
Other information