Skip to content
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

[Icon] Bye bye tabler_icon ➡️ Welcome Symfony UX Icon 🎉 #194

Open
cavasinf opened this issue Apr 24, 2024 · 9 comments · May be fixed by #195
Open

[Icon] Bye bye tabler_icon ➡️ Welcome Symfony UX Icon 🎉 #194

cavasinf opened this issue Apr 24, 2024 · 9 comments · May be fixed by #195
Labels
BC Break This will cause BC Break Feature Feature requested Status: Has PR Waiting in PR to be merge
Milestone

Comments

@cavasinf
Copy link
Collaborator

cavasinf commented Apr 24, 2024

As already discussed in #17, we've got a gap in how to handle icons in this bundle:

  • Lib Tabler icons? not really (as it should because of the theme)
  • Lib FontAwesome icon (which version? How to switch to Tabler? ...)
  • <svg>? <i>?
  • Tabler's 'icon' class that doesn't work at all with FontAwesome

Possible solution

We've been using Symfony Ux icon for the past months and it's been a total game-changer!
It works like a charm and the best part is how easy it is to switch between icon sets (Bootstrap, Tabler, FontAwesome).
And we can send the search page to our clients and let them find what they like on their own.
Furthermore, they can provide us with their SVG icons, which we can incorporate into our Symfony application.

Great feature of today's bundle icons

The best part of tabler_icon today is the shortcut/library of words that are equal to an icon:

tabler:
   icons:
      issue: fas fa-bug

Same concept with UX Icon

But we ended to reproduce this same feature with UX Icon.

  1. By "importing" (downloading) the icon:

      php bin/console ux:icons:import tabler:bug

    Downloaded at: assets/icons/tabler/bug.svg

  2. Rename it to our "shortcut" and move it to the parent icon folder:
    assets/icons/issue.svg

  3. We can now call it like this:

    • {{ ux_icon('issue') }}
    • Or with twig component: <twig:ux:icon name="issue" />

TablerBundle pros

With that external bundle, we can get out of a tight spot with "custom" code that isn't related to the Tabler theme
Plus, we get to remove code to maintain!

TablerBundle cons

  • Deprecating tabler_icon 😢

WDYT ?

@cavasinf cavasinf added Status: Needs Review Not under investigation Feature Feature requested BC Break This will cause BC Break labels Apr 24, 2024
@cavasinf cavasinf added this to the 2.0 milestone Apr 24, 2024
@cavasinf
Copy link
Collaborator Author

Pinging @tacman as he likes TwigComponent 😉

@tacman
Copy link
Contributor

tacman commented Apr 24, 2024

Indeed! The new UX Icon component is very cool.

@kevinpapst
Copy link
Owner

Do I understand that correct: the biggest benefit from this bundle perspective is, that the icon class finally works as expected? Thats great, I could remove quite some workarounds.

Even though I fear what might break 😁 but then I could document the required steps for the community.

Yes, I don't mind deprecating it, if there is an "official" replacement and the UX initiative is much better than this bundles code anyway.

The docs need to be adjusted and some deprecation trigger added. Do you want to do it @cavasinf ?

@kevinpapst
Copy link
Owner

Oh btw: what is your experience performance wise? Dozens small files are better than one large icon font file?

@tacman
Copy link
Contributor

tacman commented Apr 25, 2024

I think that because the ux components integrate with AssetMapper, the icon files are all precached and so there's no performance impact.

@kevinpapst kevinpapst linked a pull request Apr 26, 2024 that will close this issue
5 tasks
@kevinpapst
Copy link
Owner

I added a first PR draft. Works, but most icons still looks shitty in my app.
There is a huge ? for me as of now, concerning the caching of the icons.

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 26, 2024

the biggest benefit from this bundle perspective is, that the icon class finally works as expected?

icon class is made for <svg> not <i> (fontawesome) , this bundle works with <svg>
Example with btn:

.btn .icon {
  width: var(--tblr-btn-icon-size);
  height: var(--tblr-btn-icon-size);
  min-width: var(--tblr-btn-icon-size);
  margin: 0 calc(var(--tblr-btn-padding-x)/ 2) 0 calc(var(--tblr-btn-padding-x)/ -4);
  vertical-align: bottom;
  color: inherit;
}

.icon {
  --tblr-icon-size: 1.25rem;
  width: var(--tblr-icon-size);
  height: var(--tblr-icon-size);
  font-size: var(--tblr-icon-size);
  vertical-align: bottom;
  stroke-width: 1.5;
}

Works, but most icons still looks shitty in my app.

Can you share some screenshots?

There is a huge ? for me as of now, concerning the caching of the icons.

Language barrier, I don't understand this sentence at all 😄
I get it now, are your cache/size/latency tests done in dev or prod mode?

@cavasinf cavasinf added Status: Has PR Waiting in PR to be merge and removed Status: Needs Review Not under investigation labels Apr 26, 2024
@kevinpapst
Copy link
Owner

kevinpapst commented Apr 26, 2024

I get it now, are your cache/size/latency tests done in dev or prod mode?

I haven't done any tests yet, I just read the documentation which states that

Icons that have a name built dynamically will not be cached.

Bildschirmfoto 2024-04-26 um 10 06 59

Maybe that is just worded badly, but I don't want to risk 50+ additional disk reads (for tiny SVG files) for every request.
Each item of the navigation has a dynamic name and then a lot of other button elements in my UI as well.

@kevinpapst
Copy link
Owner

Just some examples... these problems are all over the app. But I guess this is just two or three places that need some fixing.

Before with Fontawesome webfont:
Bildschirmfoto 2024-04-26 um 11 09 17

With SVGs:
Bildschirmfoto 2024-04-26 um 09 44 06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break This will cause BC Break Feature Feature requested Status: Has PR Waiting in PR to be merge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants