feat(icon): add font icon support to ion-icon - #31302
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
I'm approving this with nits, but I do think the size issue is worth considering more (and may not actually be a nit if you agree it could be a problem)
| import { expect } from '@playwright/test'; | ||
| import { configs, test } from '@utils/test/playwright'; | ||
|
|
||
| configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { |
There was a problem hiding this comment.
| configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { | |
| /** | |
| * Font icon rendering does not vary across modes/directions. | |
| */ | |
| configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { |
Nit: could you add a note on why this is md/ltr only? We generally put a /** */ block above a restricted configs() call, like select-modal does. Up to you!
| .select-option-end > ion-icon { | ||
| width: 28px; | ||
| height: 28px; | ||
| font-size: 28px; |
There was a problem hiding this comment.
I think size might get past this now. ionicons applies font-size: 2rem !important for size="large" in its own shadow root, and an inner !important beats our outer rule, so a large icon measured 32px for me where the old width/height held it at 28px. The comment above mentions capping slotted content, so figured I'd flag it.
Adding !important here would keep the cap, or :not([size]) if you'd rather size win. Same in the MD file. Could be missing something though!
thetaPC
left a comment
thetaPC
left a comment
There was a problem hiding this comment.
Once you verify what Shane said, then I can move forward with my decision.
3 participants
Issue number: internal
What is the current behavior?
Ionicons added support for font icons in
ion-icon, but Ionic Framework does not support it.What is the new behavior?
ion-iconusingwidth/heightto usefont-sizeso it will style them for both SVGs and font icons.Does this introduce a breaking change?
Other information
Preview: icon: font