🇫🇷France @pdureau

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.

MR ready: https://git.drupalcode.org/project/ui_icons/-/merge_requests/56

But i still have a warning to fix:

Warning: Undefined array key 1 in Drupal\ui_icons_patterns\Plugin\UiPatterns\Source\IconSource->getPropValue() (line 29 of modules/custom/ui_icons/modules/ui_icons_patterns/src/Plugin/UiPatterns/Source/IconSource.php).

pdureau → created an issue.

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: {}

We do nothing for now. CSS variables in DSFR doesn't look very nicely implemented today, and DSFR is not a brand-agnostic deisgn system so this feature is low priority.

I see there were already a call to AttributesPropType::normalize() in LinksPropType::normalize(), but you removed some apparently useless logic executed before. I hope it was not done like that in purpose.

There are 3 phpunit failures in your MR:

• 1) Drupal\Tests\ui_patterns_field_formatters\Functional\FieldFormatterRenderTest::testRender
• Test 'field_formatter_default_2' failed for prop/slot 'props' of component ui_patterns_test:test-component. Selector .ui-patterns-props-string.
• 2) Drupal\Tests\ui_patterns_field_formatters\Functional\LayoutBuilderFieldFormatterRenderTest::testRender
• Test 'field_formatter_default_2' failed for prop/slot 'props' of component ui_patterns_test:test-component. Selector .ui-patterns-props-string.
• 3) Drupal\Tests\ui_patterns_field_formatters\Functional\LayoutFieldFormatterRenderTest::testRender
• Test 'field_formatter_default_2' failed for prop/slot 'props' of component ui_patterns_test:test-component. Selector .ui-patterns-props-string.

Is it related to your change?

Also, maybe we can also move some string related logic (like ::normalizeObject()) from AttributesPropType::normalize() to StringPropType::normalize()

i ill merge as soon as the pipeline is run

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

We don't allow render arrays in prop values, so in links item titles.

Instead of skipping the cast to string, we may render the renderable instead.

Let's discuss

g4mbini → credited pdureau → .

Let's resume this task.

@christian.wiederman: can you rebase your work and check if the pipelien is OK after the rebase?

Once done, I will redo the tests with Experience Builder.

yes, please, add the kernel tests because I never done such a thing

Let's fix that in an otehr issue:

Each enum_set (priority: 10) is also a enum_list (priority: 5) which is also a list (priority: 1)

phpcs error:

 89 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "NULL" but
    |       |     found "null" (Generic.PHP.UpperCaseConstant.Found)

Also, you need to rebase.

grimreaper → credited pdureau → .

grimreaper → credited pdureau → .

I have added a few coding related comments, but my main concern is the logic being located in ComponentElementBuilder instead of ComponentElementAlter.

Because those 2 methods are only called from ComponentElementAlter for now, and we want to keep the calls here so all renderables will benefit from the logic instead of only the renderables built from ComponentElementBuilder .

Am I missing something?

run the twig linter on existign codebase

Done in beta5: https://git.drupalcode.org/project/ui_patterns/-/commit/427b83660dffb549...

add the linter to our CI pipeline

Moved to RC scope.

Another branch, so another MR, with another proposal: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/267

Instead of calling the render element from include function, include tag and embed tag, we stay closer to the current SDC implementation:

1. reorganize our Twig modules visitors to be run before and after the SDC one
2. move some logic from the render element to an internal Twig function run by one of the Twig modules visitors
3. promote include() function as a replacement of component() function

Why include() function? Because:

• include tag is not recommended by the Twig documentation: bhttps://twig.symfony.com/doc/3.x/tags/include.html
• as previously stated, embed tag is an overcomplicated, Twig specific, template altering mechanism which must be avoided at any cost

The crazy, ambitious, change is not forgotten and will be discusse din the Core issue: 📌 Trigger SDC render element when using Twig include or embed Active

Finally, the expected snippet is:

{% set content = content and content is not sequence ? [content] : content %}

pdureau → changed the visibility of the branch 3449653-only-twig-visitor to active.

pdureau → changed the visibility of the branch 3449653-only-twig-visitor to hidden.

Another issue.

Overlaping between slots & props

For example, card:

props:
  type: object
  properties:
    card_img_top:
      type: string
    card_header:
      type: string
    card_body:
      type: string
    card_footer:
      type: string
slots:
  card_img_top_block:
    title: Image
  card_header_block:
    title: Card Header
  card_body_block:
    title: Card Body
  card_footer_block:
    title: Card Footer

In the template, we have this weird structure:

  {% if card_header or block('card_header_block') %}
    <div class="card-header">
      {% block card_header_block %}
        {{ card_header }}
      {% endblock %}
    </div>
  {% endif %}

Where something like that must be enough:

  {% if card_header %}
    <div class="card-header">
        {{ card_header }}
    </div>
  {% endif %}

There is a global feeling of lack of confidence with the slot management, because of the duplication between props & slots, but also because of the naming: why prefixing props and slots by the component name, and suffixing the slots by "_block"?

grimreaper → credited pdureau → .

grimreaper → credited pdureau → .

grimreaper → credited pdureau → .

Florent, I think we can merge like that because for "UI Styles" we will explore an other strategy:

• add a Style source plugin for attributes props
• expects components authors to crate one attributes prop by region (slot) when implementing a layout/grid system with SDc, like done by ✨ Create patterns/components for grid management Active in UI suite Bootstrap
  

      col_1_attributes:
        title: "Column attributes"
        description: "The attributes to customize the tag with the col class."
        $ref: "ui-patterns://attributes"
      col_2_attributes:
        title: "Column attributes"
        description: "The attributes to customize the tag with the col class."
        $ref: "ui-patterns://attributes"
  

• deprecate some parts of ui_styles_layout_builder?

Do we continue this discussion in a UI Style issue?

Now SDC in in the Drupal\Core\Theme namespace, the final keyword was removed and it is possible to decorate the plugin manager (Experience builder is already doing that).

The main obstacle for extensibility is now ✨ ComponentPluginManager must implement CategorizingPluginManagerInterface Active because a service decorator can't add an interface to the service.

Remove everything but ::getNodeVisitors() in ComponentsTwigExtension and most of ComponentNodeVisitor

It would also be the opportunity to also remove ComponentElement::generateComponentTemplate() and to simply render:

[
   '#type' => 'inline_template',
   '#template' => $template_loaded_from_file,
  '#context' => $slots + $props,
];

First proposal for include: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/262/diffs

It was the easy one. embed will be an harder challenge

Ok, i will give a try

I have also run the Twig linter

pdureau → created an issue.

Do we need to also update the examples from UI Examples ?

pdureau → created an issue.

Done: https://git.drupalcode.org/project/ui_suite_dsfr/-/merge_requests/123

If we want to add other attributes props (one by region), like Florent is currently doing with UI Suite Bootstrap, let's do that in a separate issue.

May be a breaking changes because identifier is stricter than string.

We also need to update grid_row to the new structure

Ok, let's do it.

I don't reproduce this on my environment. This is an enum_list:

    offset_xl:
      title: "Columns offset (extra large)"
      type: array
      maxItems: 3
      items:
        type: integer
        enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]

This is an enum_set:

    test:
      title: "Test"
      type: array
      uniqueItems: true
      maxItems: 3
      items:
        type: integer
        enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] 

and put priority: 1 to EnumList prop type in its attributes

But we need to do some changes because it is better and safer to set priority attribute instead of relying on plugin discovery order, but 1 is already the default value and let's update both enum_set and enum_list:

• Each enum (priority: 10) is also a string (priority: 1) or a number (priority: 1)
• Each identifier (priority: 100) is also a string (priority: 1)
• Each url (priority: 10) is also a string (priority: 1)
• So, each enum_set (priority: 10) is also a enum_list (priority: 1)

Once the change done, CompatibilityChecker must also be updated. Thanks for your proposal, it works well.

Grimreaper, what do you think about the current state of work? https://git.drupalcode.org/project/ui_patterns/-/merge_requests/261/diffs

It is not finished, I will need to inject the plugin.manager.component_story dependency for example, but it works and it is advanced enough to talk:

• is TwigExtension the best place to put the logic?
• We trigegr the logic only for single component library page ( ui-patterns-stories-full.html.twig) ?
• _story or story for the variable injected in library_wrapper ?
• ...

Thanks you. Changes done.

I was not sure about the naming of the trait, so I did "EnumTrait"

Let's try to not use the word "token" in UI Suite.

Tokens are already resolved at buildtime and don't exist anymore in the artefacts we are implementing (components, styles..)

Tokens are sass variables for example.

grimreaper → credited pdureau → .

Update the management of enumeration in both widgets and prop types

DONE

Current status.

OOS:

• Test SelectsWidget with 📌 [1.1.0] From Layout Options to UI Patterns 2.x Active >> DONE
• Do we put select title only when minItems? >> NO, always, for accesibility
• If possible: Dynamic number of selects elements when no maxItems >> NO, let's keep thing simple

TODO:

• Update the management of enumeration in both widgets and prop types
• Update schema compatibility checker?
• Unit tests?

Proposal based on 📌 [2.0.0-beta5] Add new EnumSetPropType Active : https://git.drupalcode.org/project/ui_suite_dsfr/-/merge_requests/122

If you are OK with this proposal, let's discuss before merging:

• Because 📌 [2.0.0-beta5] Add new EnumSetPropType Active neeeds to be merged before
• We also need to update grid_row to the new structure

Yes, it is a SDC issue, and Core team has the same worries as you: 🐛 SDC: Make empty render arrays evaluate to false in component templates Active

But it is also doable at a UIP2 level IMHO, and we can propose to move the fix to Core later

Side note: on the actual MR (so before my proposal with enum_list, offset values were wrong it some enums fr-col-<code> instead of <code>fr-col-offset-

So, it looks OK on our side.

Work in progress. Remaining work to do:

• Test SelectsWidget with 📌 [1.1.0] From Layout Options to UI Patterns 2.x Active
• If possible: Dynamic number of selects elements when no maxItems
• Update schema compatibility checker?
• Unit tests

So, everything is OK on our side.

So, we may need this UIP2 issue to be completed first: 📌 [2.0.0-beta5] Add new EnumSetPropType Active

Because, once coupled with the new SelectsWidget source, the enum_list prop may be a perfect fit for list of values based on the same enum.

For example, those 10 enum props:

    fr_col_first:
      title: "Column span first"
      type: string
      enum: [fr-col-1, fr-col-2, fr-col-3,  fr-col-4, fr-col-5, fr-col-6, fr-col-7, fr-col-8, fr-col-9, fr-col-10, fr-col-11, fr-col-12]
    fr_col_first_sm:
      title: "Column span first (small)"
      type: string
      enum: [fr-col-sm-1, fr-col-sm-2, fr-col-sm-3,  fr-col-sm-4, fr-col-sm-5, fr-col-sm-6, fr-col-sm-7, fr-col-sm-8, fr-col-sm-9, fr-col-sm-10, fr-col-11, fr-col-sm-12]
    fr_col_first_md:
      title: "Column span first (medium)"
      type: string
      enum: [...]
    fr_col_first_lg:
      title: "Column span first (large)"
      type: string
      enum: [...]
    fr_col_first_xl:
      title: "Column span first (extra large)"
      type: string
       enum: [...]
    fr_col_second:
      title: "Column span second"
      type: string
      enum: [fr-col-1, fr-col-2, fr-col-3,  fr-col-4, fr-col-5, fr-col-6, fr-col-7, fr-col-8, fr-col-9, fr-col-10, fr-col-11, fr-col-12]
    fr_col_second_sm:
      title: "Column span second (small)"
      type: string
      enum: [fr-col-sm-1, fr-col-sm-2, fr-col-sm-3,  fr-col-sm-4, fr-col-sm-5, fr-col-sm-6, fr-col-sm-7, fr-col-sm-8, fr-col-sm-9, fr-col-sm-10, fr-col-11, fr-col-sm-12]
    fr_col_second_md:
      title: "Column span second (medium)"
      type: string
      enum: [...]
    fr_col_second_lg:
      title: "Column span second (large)"
      type: string
      enum: [...]
    fr_col_second_xl:
      title: "Column span second (extra large)"
      type: string
      enum: [...]

could be replaced by those 5 enum_list props:

    cols:
      title: Cols
      type: array
      maxItems: 2
      items:
        type: integer
        enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]
    cols_sm:
      title: Cols SM
      type: array
      maxItems: 2
      items:
        type: integer
        enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]
    cols_md:
      title: Cols MD
      type: array
      maxItems: 2
      items:
        type: integer
        enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]
    cols_lg:
      title: Cols LG
      type: array
      maxItems: 2
      items:
        type: integer
        enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]
    cols_xl:
      title: Cols XL
      type: array
      minItems: 2
      maxItems: 2
      items:
        type: integer
        enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]

pdureau → created an issue.

This is something we will need to change. And it will be the good opportunity to update grid_row too. But for now, we do nothing until I fixed the UIP2 issue

(Sidenote for later: why prefixing so much props with "fr_"?)

I will take time doing this review because, looking at your props, I may have found a little something to change in UIP2 prop types