🇫🇷France @mogtofu33

Ok, so for now, just added a ui_icons_font for the dompdf dependency, let see if we really need one main.

mogtofu33 → created an issue.

g4mbini → credited mogtofu33 → .

Thanks for the work!

I took some of the code here to fix the menu and allow the admin display.

For the non menu link content, I checked and cannot find a solution for the serialization on the preview, perhaps the FormElement with autocomplete is simply not compatible here.

As there is the workaround to manually create the same menu for non content link, I think it's less important for beta3 and can be pushed later.

I added a cache collector for review.

In the meantime I fixed a memory footprint of loading thousands of icons in the pack definition, before each icon was an Object IconDefinition, now it's a simple array with minimal data. Plugin extractor must implement loadIcon()to return the object which is a big improvement on the cache bin size, around 8 times smaller.

Looks good, thanks!

mogtofu33 → created an issue.

mogtofu33 → created an issue.

mogtofu33 → made their first commit to this issue’s fork.

mogtofu33 → made their first commit to this issue’s fork.

I will add the viewbox available as a variable if set in the svg, this will match your needs and be an optional feature when people need it.

I will update here when pushed on the main MR.

@mxh, as highlighted by @catch, each icon is not a plugin; the icon pack itself is a plugin containing all extracted icons.

Note: We've used the term "Extractor" instead of "discovery" to avoid confusion with Drupal's core discovery mechanism. If someone have a better suggestion, please let me know.

The icon pack leverages the discovery mechanism to cache the list of available icons (DefaultPluginManager).

Current Behavior for IconPackManager works as follows:

• Load Definitions: Loads icon definitions from *.icons.yml files.
• Process Definitions: Runs the extractor plugin to generate a list of available icons for each definition.
• Cache Definitions: Caches the processed definitions, including the icon list, in the discovery cache.

Later when getIcon is called:

• Load Icon List: Retrieves the cached icon list from the discovery cache.
• Find Matching Icon: Locates the requested icon (optimized for faster lookup on the first call).
• Render Icon: Renders the icon using an inline inline_template based on the definition template.

Potential Optimization:

While we could optimize getIcon by caching a separate list of frequently used icons, the performance gain would be minimal, especially considering the relatively infrequent calls to getIcon and the efficiency of the current caching mechanism.

Even with multiple definitions and thousands of icons, the impact of extracting and rendering a few dozen icons on a page is negligible.

But I am open to any optimization, I will try to profile a second cache to get more specific numbers.

I think it's probably a duplicate of ✨ Menu: support non content menu links Active , will wait a bit before closing when I'll look at it.

So it's not a problem with ui_icons but ui_suite_daisyui, issue created and fix pushed for review on ui_suite_daisy ui: 🐛 Icons pack template error and settings. Active .

mogtofu33 → created an issue.

Could you give steps to reproduce?

Hi @quietone,

Thanks for the fixes and review!

I've improved the comment in `core/lib/Drupal/Core/Theme/Icon/Plugin/IconPackManager.php` to provide more clarity.

Additionally, I created a documentation page on the original UI Icons module: [here](https://project.pages.drupalcode.org/ui_icons/#examples). I will add this documentation (with some updates) into the official Drupal documentation once this merge is accepted.

Please let me know if you think it would be helpful to include further details in the comment, such as information on settings, template usage, or a preview mechanism.

mogtofu33 → created an issue.

mogtofu33 → made their first commit to this issue’s fork.

TwigFunction recommendation is wrong as well, only the test setup seems missing.

The recommended createMock() replacment seems an error, see 🐛 Unclear replacement of getMockBuilder() with createMock(). Active

We still can consider:

--- a/src/Template/IconTwigExtension.php
+++ b/src/Template/IconTwigExtension.php
@@ -23,8 +23,8 @@ class IconTwigExtension extends AbstractExtension {
    */
   public function getFunctions(): array {
     return [
-      new TwigFunction('icon', [$this, 'getIconRenderable']),
-      new TwigFunction('icon_preview', [$this, 'getIconPreview']),
+      new TwigFunction('icon', $this->getIconRenderable(...)),
+      new TwigFunction('icon_preview', $this->getIconPreview(...)),
     ];
   }
<code>

<code>
--- a/tests/src/Unit/Template/IconTwigExtensionTest.php
+++ b/tests/src/Unit/Template/IconTwigExtensionTest.php
@@ -34,6 +34,7 @@ class IconTwigExtensionTest extends TestCase {
    * {@inheritdoc}
    */
   protected function setUp(): void {
+    parent::setUp();
     $this->pluginManagerIconPack = $this->createMock(IconPackManagerInterface::class);
     $this->iconTwigExtension = new IconTwigExtension($this->pluginManagerIconPack);
   }

Could you be more specific with step to reproduce? What didi you do to have a working and a non working Icon?

On CKEditor as you've seen the icon tag is <drupal-icon>, not the svg. The svg HTML is rendered dynamically from a js call on a specific route like with settings : /ui-icons/icon/preview?icon_id=bootstrap%3Agoogle&settings=%7B%22size%22%3A%2264%22%2C%22color%22%3A%22%23000000%22%2C%22alt%22%3A%22%22%7D

You can check the browser console to see the xhr click right > open as new Tab (For firefox). And see if the generated HTML is correct.

mogtofu33 → created an issue.

Thank you @larowlan, for the initial code review.

The suggested changes have been merged, and most of the comments have been addressed. Only a few discussions remain.
The current failing functional test is not related to the Icon code but to the LayoutSectionTest. I'm unsure if we need to address this.

Thanks for the work, looks great to me.

As I get caching is applied only for IconPackManager definitions so the rest of API will use file-search for every icon requested, hope I'm wrong...

@andypost, the result of the file search (when it's a file search based icon) is added to the `IconPackManager` definition, and then cached with it. Each icon request do not involve the file search again.

I see no real usage of API (except unit tests), as there's render and forms fucntional test required, moreover it needs perf test to make sure the page is renderable when there's 100-200 icons

On the perf side, when used with render api or twig function, it's just an inline template, no call to database.
As mentioned by @nod_ we will propose soon an implementation with core navigation module.

Thanks for the fix!

Little changes with ui_icons beta2 → about the name of the hook (we changed `ui_icons` to `icon` to ease the core merge we hope for).
So when hopefully it goes core you will not have to change anything.

Second change is about adding processDefinition(), because in the class DefaultPluginManager, method alterDefinitions() run after processDefinition(), and we use the process to call the extractor for the list of icons discovered. So because you add a definition you need to have the process step added.

mogtofu33 → made their first commit to this issue’s fork.

mogtofu33 → created an issue.

Current fork was messy, moved as mr: https://git.drupalcode.org/project/ui_icons/-/merge_requests/41

mogtofu33 → made their first commit to this issue’s fork.

Fixed on 🐛 Autocomplete element theme Needs work

Fixed on 🐛 [1.0.0-beta2] Autocomplete display Needs work

mogtofu33 → made their first commit to this issue’s fork.

g4mbini → credited mogtofu33 → .

mogtofu33 → created an issue.

mogtofu33 → created an issue.

mogtofu33 → created an issue.

mogtofu33 → created an issue.

Not sure if this could be simply a double quote error?

Cannot reproduce on a fresh install 10.3.5/11.0.4.

I don't see anything in the logs, is it an existing field from alpha version? There is no update path from alpha to beta other than a flush caches and re-install is using an example module.

mogtofu33 → created an issue.

Still has the focus issue, will move to ne issue of less priority.

Thanks for the report.

Seems there is a wrong cleanup on unused service, but sill it's declared in *.services.yml so a cache clear is enough to fix, will remove service.

Work nicely without anything specific, see UI Icons DSFR.

mogtofu33 → created an issue.