[1.1.x] Adopts the new Icon API

i will take it.
@pdureau, do you suggest that we should wait before adopting ui_icons?

like waiting for the core MR to be merged or waiting for a stable version of ui_icons module?

@pdureau, do you suggest that we should wait before adopting ui_icons?

It is a breaking change for UI Suite DSFR, so we need to do it for 1.1.0, not later. We will not introduce breaking changes in patch releases (1.1.1, 1.1.2, 1.1.3....).

Today, the icon prop type and its related source plugin are found in this module: https://git.drupalcode.org/project/ui_icons/-/tree/1.0.x/modules/ui_icon.... But it will move in https://git.drupalcode.org/project/ui_patterns/-/tree/2.0.x/src/Plugin/U... as soon as ✨ Add an icon management API Active is merged.

The plugin ID will not change, it will stay icon, so we can start with https://git.drupalcode.org/project/ui_icons/-/tree/1.0.x/modules/ui_icon... (let's not forget the temporary dependency)

we also need to remove the corresponding style utility (in ui_suite_dsfr.ui_styles.yml) ? And what about the related icon size utility?

Just a little side note before the review: I believe it is not necessary to put the empty setting value in stories:

     type: icon
     icon_pack: dsfr
    settings: {}

Also, the empty valus is not compliant with our linter prettier:

-          settings: { }
+          settings: {}

So, the actual review.

---------------------

Careful, to follow the changes from ✨ Add an icon management API Active , the icon prop type structure has changed, from:

      'icon_pack' => ['$ref' => 'ui-patterns://identifier'],
      'icon' => ['type' => 'string'],
      'settings' => ['type' => 'object'],

to:

      'pack_id' => ['$ref' => 'ui-patterns://identifier'],
      'icon_id' => ['type' => 'string'],
      'settings' => ['type' => 'object'],

https://git.drupalcode.org/project/ui_icons/-/commit/87e8ed2b5962a21fe58...

-------------

You have set 3 icon packs:

• dsfr, using SVG extractor and printing a SVG element
• dsfr_picto, using SVG extractor and printing a SVG element
• and dsfr_name (disabled), using SVG extractor and printing a SPAN element, with the same icons as dsfr!

So, code>dsfr_picto is OK but you need to chose between dsfr & dsfr_name because they are the same set of icons with different rendering.

When someone will use the icon prop type in a component, they will have to pick an icon from one of the packs, and they will have a settings form to fill, what they set in the form need to have an impact on what happen in the template:

• If you decide to keep using the icons like that in the template: attributes.addClass('fr-icon-' ~ icon.icon) I would chose to keep dsfr_name (which can be renamed dsfr) because its settings can be used of the template, but the settings of the actual dsfr (size, color and alt) can't be used.
• For dsfr_picto it is more complicated because the SVG element is the only way of rendering this icon pack according to upstream documentation, so we need a way to exclude code>dsfr_picto from the prop source.

I assigned to you for the dsfr_name & dsfr change, but for the dsfr_picto we will need to fo this first: ✨ Use #allowed_icon_pack with UI Patterns 2 Active

-------------------------

Other little feedbacks:

• You kept the icon style utility?
• You don't need the dependency to ui_icons:ui_icons if you have a dependency to ui_icons:ui_icons_patterns
• The icon packs descriptions are not very clear. Maybe you can copy and translate the descriptions form upstream documentation.

thanks for all the precious feedbacks.

after a few tries, i may have figured it out.

i will post a few updates

i just rebased.
i forgot yesterday to indicate the status "needs review".
Pierre, would you please review?

Ideally, we will require ui_icons 1.0-beta3 as a minimal version (to be published in a few days)

Currently reviewing.

First feedbacks..

you don't need to put settings in stories each time:

  detail_icon:
    type: icon
    pack_id: dsfr
    settings: { }
    icon_id: file-pdf-line

this snippet is OK and repeated many times:

{% if icon.icon_id %}
  {% set attributes = attributes.addClass([icon.icon_id ? 'fr-icon-' ~ icon.icon_id|clean_class, (icon.settings.size|default('md') != 'md') ? 'fr-icon--' ~ icon.settings.size|clean_class, icon.settings.color ? 'fr-text-' ~ icon.settings.color|clean_class]) %}
{% endif %}

 {% set detail_end_attributes = detail_end_attributes.addClass(['fr-card__detail', detail_end_icon.icon_id ? 'fr-icon-' ~ detail_end_icon.icon_id|clean_class, (detail_end_icon.settings.size|default('md') != 'md') ? 'fr-icon--' ~ detail_end_icon.settings.size|clean_class]) %}
<div{{ detail_end_attributes }}>{{ detail_end }}</div>

{% if not dismissible and icon.icon_id %}
  {% set attributes = attributes.addClass([icon.icon_id ? 'fr-icon-' ~ icon.icon_id|clean_class, (icon.settings.size|default('md') != 'md') ? 'fr-icon--' ~ icon.settings.size|clean_class, icon.settings.color ? 'fr-text-' ~ icon.settings.color|clean_class]).addClass('fr-tag--icon-left') %}
{% endif %}

Let's see if we can make this more redable...

You chose to put the icon packs in /libraries/. Maybe it can be explained in a README.md (or similar) file.

The position of the library in /libraries/ is already enforced :)

see file ui_suite_dsfr.libraries.yml

Issue is rebased :)