[ux-icon] Using ux_icon through tabler_icon#247
[ux-icon] Using ux_icon through tabler_icon#247cavasinf wants to merge 14 commits intokevinpapst:mainfrom
ux_icon through tabler_icon#247Conversation
cavasinf
added
Status: Needs Review
|
Nice, but what was wrong with the old PR #195 ? |
|
I’m trying to make this as “no changes needed” as possible for other developers. |
cavasinf
changed the title
tabler_icon by ux_iconux_icon through tabler_icon
| {% macro item_icon(item) %} | ||
| {% if item.icon %} | ||
| <span class="nav-link-icon d-md-none d-lg-inline-block text-center">{{ tabler_icon(item.icon, false, item.icon) }}</span> | ||
| <span class="nav-link-icon d-md-none d-lg-inline-block text-center">{{ tabler_icon(item.icon, true, item.icon) }}</span> |
There was a problem hiding this comment.
Because this is now the correct way to use the .icon class with Symfony UX Icons, which render SVGs instead of webfonts.
So the icon class has always been required on menu icon elements from the start.
There was a problem hiding this comment.
I thought so. This is a rather visually unacceptable BC break for everyone, who cannot immediately switch to ux-icons, like me... Can we either make this somehow configurable OR provide a CSS snippet to fix the problem?
Looks worse, when its not so zoomed
There was a problem hiding this comment.
Looks worse, when its not so zoomed
Wait, this behavior happens when the .icon class is used with webfonts.
In your screenshot, is it a webfont or an SVG?
With this PR, all <i> elements are replaced by <svg>, so the zoom effect should no longer occur.
| "symfony/twig-bridge": "^6.0 || ^7.0 || ^8.0", | ||
| "twig/twig": "^3.0" | ||
| "twig/twig": "^3.0", | ||
| "symfony/ux-icons": "^2.0" |
There was a problem hiding this comment.
We should move symfony/ux-icons to suggest section and make the dependency optional.
I will see if I find a way.
There was a problem hiding this comment.
I was doing that at the beginning, but since we allow icon alias references from MenuItem, we NEED to be able to generate the icon and not only recommend it.
That said, my idea would be to allow developers to pass an html string to that parameter. Why?
- It gives them full control over what is rendered in the menu
- They are not forced to use FontAwesome (as today) or a future
ux-iconsolution
TBH, I was thinking the same approach could apply to every component that accepts an icon alias. We should only accept an HTML string (still BC-compatible if an alias is provided) and simply render icon|raw inside the component.
For your example with that f***ing icon class, you will be able to add/remove it yourself, and not being forced by the menu template.
There was a problem hiding this comment.
We could make it required now, and suggest it for 3.0 later
Co-authored-by: Kevin Papst <kevinpapst@users.noreply.github.com>
2 participants
Description
Types of changes
Checklist