Paris
Account created on 13 April 2012, almost 12 years ago
  • Engagement Manager at SmileΒ 
  • Business Unit Manager at LinagoraΒ 
#

Merge Requests

More

Recent comments

πŸ‡«πŸ‡·France pdureau Paris

After rebasing πŸ“Œ [2.0.X-alpha1] Introduce buildComponentRenderable Needs review , there are a few issues left:

  • variant selector is not working: variant selection is not saved and retrieved
  • after refreshing the page, all the form is reset expect the component selector
πŸ‡«πŸ‡·France pdureau Paris

Hello,

we should use drupal mechanisms path() or ur() instead of hard href so url can be altered by Drupal core, contrib and custom modules.

We shouldn't use those functions (path() & url()) in a component templates. They are stateful and application related because they do DB query to get routing information.

However, I understand you are not comfortable with hardcoding "/" in the template. A better solution would be to add a new URL prop ("settings") in https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/1.0.x/templates/... :

  settings:
    home_url:
      type: url
      label: Homepage URL
      preview: /

Use it in https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/1.0.x/templates/... :

 {% if home_url and (not operator_logo) and (not service_title) %}
  <a href="{{ home_url|default("/") " title="{{ 'Homepage'|t }} - {{ logo_text }}">
{% endif %}
...

And then you can use path() or url() from https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/1.0.x/templates/... :

  {{ pattern('header', {
    'logo_text': page.logo,
    'home_url': path('<front>'),
    'operator_logo': page.header_operator_logo,
    'service_title': site_name,
    'service_tagline': site_slogan,
    'tools_links': page.header_tools_links,
    'tools_search': page.header_tools_search,
    'navbar': page.header_navbar,
  }) }}
πŸ‡«πŸ‡·France pdureau Paris

Also: add default "value" key in each source plugin forms.

Like content field properties, there is always at least a key, and this one is "value" by default.

πŸ‡«πŸ‡·France pdureau Paris

Pierre: review the MR & refactor legacy stuff as well

πŸ‡«πŸ‡·France pdureau Paris

If there is a class starting with "usa-logo", it belongs to the component.

Stuff without a class starting with "usa-logo":

  • belongs to the site branding template if outside "usa-logo"
  • is the content of a slot if inside "usa-logo"

My proposal from https://www.drupal.org/project/ui_suite_uswds/issues/3417850#comment-154... πŸ› Logo component do too much Needs review was not OK?

πŸ‡«πŸ‡·France pdureau Paris

i didn't tested locally, but the PR looks good

πŸ‡«πŸ‡·France pdureau Paris

FYI, Florent is working on ✨ Make the Twig loops safer Postponed for UI Patterns 1.x

Once done, my current proposal:

      {% set is_list = meta_items_list is iterable and meta_items_list|keys|first == 0 %}
      {% set is_children = meta_items_list is iterable and meta_items_list|keys|first|first != "#" %}
      {% set meta_items_list = is_list or is_children ? meta_items_list : [meta_items_list] %}
      {% for meta_item in meta_items_list -%}

Can be replaced by:

      {% set meta_items_list = meta_items_list is sequence ? meta_items_list : [meta_items_list] %}
      {% for meta_item in meta_items_list -%}
πŸ‡«πŸ‡·France pdureau Paris

Review done. I would be happy to continue this conversation if you still need me.

πŸ‡«πŸ‡·France pdureau Paris

Heading component

https://git.drupalcode.org/project/radix/-/tree/6.0.x/components/heading

Unused attributes variable

heading_attributes is used instead of the default attributes variable.

It is defined in the YML (like badge component do for badge_attributes, or page-footer do for footer_attributes) but it would be better to use the default attributes variable.

And it will prevent this kind of verbose precautions:
{% set heading_attributes = heading_attributes ?: create_attribute() %}

Duality slots / props

So, now we have heading_content slot as a Twig block AND title_link & content props inside the exact same block:

    {% block heading_content %}
      {% if title_link %}
        <a href="{{ title_link }}">{{ content }}</a>
      {% else %}
        {{ content }}
      {% endif %}
    {% endblock %}

What will happen if we call the component with only the slot ?

"#type" => "component",
"#component"  =>"radix:heading",
"#slots" => [
  "content" => ["#plain_text" => "Lorem ipusm"],
]

Only the props ?

"#type" => "component",
"#component"  =>"radix:heading",
"#props" => [
  "title_link" => "Foo",
  "content" => "Bar",
]

Both slot and props ?

Let's be careful with the Twig block system. It is useful for template inheritance, but in component driven development we don't do template inheritance.

nav-item component

https://git.drupalcode.org/project/radix/-/tree/6.0.x/components/nav-item

Unused attributes variable

nav_item_attributes is used in template but not defined anywhere.
But as seen for heading component, instead of adding the prop in the definition, it is better to use the default attributes variable.

link slot

The template looks now OK:
{{ link|add_class('nav-link', is_active ? 'active' : '') }}

But not the definition yet. It went from undefined object to undefined array.

Accordion component

Not reviewed yet. I will update this comment later.

Table component

table prop is now split in 5 slots (table_caption, table_colgroup, table_header, table_body & table_footer) and 5 props:

  • caption: String
  • colgroups: Undefined array
  • header: Undefined array
  • rows: Undefined array
  • footer: String

Like for heading component, what to do with duality slots/props ?

Array of HTML classes

Perfect :)

title_prefix & title_suffix

Indeed, they are not always slots. Fro example, {{ title_suffix.contextual_links }} in media component.
But they need to be defined anywhere. What is expected inside ?

πŸ‡«πŸ‡·France pdureau Paris

Pushed. Not fully tested. I hope it is OK.

πŸ‡«πŸ‡·France pdureau Paris

I will have a look

πŸ‡«πŸ‡·France pdureau Paris

Enough is enough :)

Let's add those 2 tests in UI Patterns 1.x and 2.x now instead of waiting https://github.com/twigphp/Twig/issues/3828 is ready, let's implement is sequence and is mapping tests directly in UI Patterns 1.x & 2.x. We will remove them once implemented upstream.

We already sure about the API to add, because it is already available in Jinja and other template engines.

However, is sequence test is not enough when a list of renderables has mapping keys (non consecutive, strings) instead of sequence (integer, consecutive) keys.

For example a list of blocks from page layout or layout builder: each block is keyed by its UUID.

So, normalize this list of renderable to be a proper Twig sequence from the pattern (or component in UI Patterns 2.x) render element itself, as a prerender function, on the first level of each slots, no recursivity.

I will do the 2.x implementation in an other issue. Someone has to take care of 1.x implementation here.

πŸ‡«πŸ‡·France pdureau Paris

Hi Stephen,

I don't think this new Twig function is the solution at our problem. get_list_values() is not a "twigy" solution. Implementing this, we will diverge too much from upstream.

If I consider your exact use case from πŸ“Œ Figure out a solution for rendering lists Needs work :

     {% set is_list = meta_items_list is iterable and meta_items_list|keys|first == 0 %}
      {% set is_children = meta_items_list is iterable and meta_items_list|keys|first|first != "#" %}
      {% set meta_items_list = is_list or is_children ? meta_items_list : [meta_items_list] %}

The first part is already covered by this issue: ✨ Make the Twig loops safer Postponed The idea is to replace (meta_items_list is iterable and meta_items_list|keys|first == 0 ) by is sequence.

The second part can be solved a prerender of the variable before sending the data to the template.

So, let's move the discussion to the other issue () with those updates:

  • instead of waiting https://github.com/twigphp/Twig/issues/3828 is ready, let's implement is sequence and is mapping tests directly in UI Patterns 1.x & 2.x. We will remove them once implemented upstream.
  • because those 2 new tests will not be enough when a list of renderables has mapping keys (non consecutive, strings) instead of sequence (integer, consecutive) keys (for example a list of blocks from page layout or layout builder), normalize this list of renderable to be a proper Twig sequence from the pattern (or component in UI Patterns 2.x) render element itself, on the first level of each slots, no recursivity.
πŸ‡«πŸ‡·France pdureau Paris

OOps, there was a typo in templates/base/misc/status-messages.html.twig

     {% for message in messages %}
       {{ pattern('alert', {
         'variant': type == 'status' ? 'info' : type,
-        'message': messages,
+        'message': message,
         'attributes': attributes.addClass('margin-y-2')
       }) }}
     {% endfor %}

I will push the tiny fix as soon as i can.

πŸ‡«πŸ‡·France pdureau Paris

Hi Stephen,

Looks quite good to me. Is its possible to use dependency injection instead of calling \Drupal::theme() ? Twig extension os a service.

Also, let's be extra careful before extending Twig in UI Patterns. I still can't believe than such a feature is not part of Drupal Core.

Drupal theme manager is injecting directory variable to all Twig templates, but there are 2 issues about it:

  • it seems it doesn't print the path of the theme or module that the template is found in, but only the path of the current theme
  • it doesn't look the base path, so people do that: {{ url('') ~ directory }} which is ugly and risky
  • It will not work with UI Patterns 2.x because SDC templates are not passing through the theme manager

So, can you propose your new function at a Drupal Core level first? It would be great to have the feedbacks of the core maintainers.

If we decide to implement it at UI Patterns level, let's do it for both 1.x & 2.x.

πŸ‡«πŸ‡·France pdureau Paris

Thanks for you draft Sorlov. Can you do a MR instead of patch once your work is done?

πŸ‡«πŸ‡·France pdureau Paris

Hi Duaelfr, I take care of 1.x reviews, and I assign the 2.x reviews to you.

πŸ‡«πŸ‡·France pdureau Paris

Hi Duaelfr, I take care of 1.x reviews, and I assign the 2.x reviews to you.

πŸ‡«πŸ‡·France pdureau Paris

Hi Duaelfr, I take care of 1.0.x reviews, and I assign the 2.0.x review to you.

πŸ‡«πŸ‡·France pdureau Paris

Hi Duaelfr, I take care of 1.0.x reviews, and I assign the 2.0.x review to you.

πŸ‡«πŸ‡·France pdureau Paris

Hi Duaelfr, I take care of 1.0.x reviews, and I assign the 2.0.x review to you.

πŸ‡«πŸ‡·France pdureau Paris

Hi Duaelfr, I take care of 2.0.x reviews, and I assign the 1.0.x review to you.

πŸ‡«πŸ‡·France pdureau Paris

Hi Duaelfr, I take care of 2.0.x reviews, and I assign the 1.0.x reviews to you.

πŸ‡«πŸ‡·France pdureau Paris

thanks Stefen, i will have a look on this

πŸ‡«πŸ‡·France pdureau Paris

this one was resolved by the new interface:

Last question: "ContextAwarePluginInterface methods are not used by the Form Builder, only internally by SourcePluginBase. is it normal ?"

πŸ‡«πŸ‡·France pdureau Paris

I didn't have the time to do everything i wanted, but that's enough for this issue which is becoming big.

Merge request is ready.

πŸ‡«πŸ‡·France pdureau Paris

New commit. Is it better?

πŸ‡«πŸ‡·France pdureau Paris

DONE:

  • Code sniffer
  • Restore buildComponentData and rename it buildComponentRenderable
  • Slots before props in form builder
  • Restore source_id in ComponentFormPropsBuilder

TODO ComponentFormPropsBuilder:

  • Extract protected static function buildPropForm(FormStateInterface $form_state, string $prop_id, array $definition, array $configuration, array $contexts): arraycalled from $element[$prop_id] = self::buildPropForm($form_state, $prop_id, $prop, $configuration[$prop_id] ?? [], $contexts);
  • Extract protected static function buildSourceSelector(FormStateInterface $form_state, string $prop_id, string $wrapper_id, array $sources, SourceInterface $selected_source): array called from $element['dropdown_actions'] = self::buildSourceSelector($form_state, $prop_id, $wrapper_id, $sources, $selected_source);

TODO ComponentFormPropsBuilder:

  • Extract protected static function buildSlotForm(FormStateInterface $form_state, string $slot_id, array $definition, array $configuration, array $options): array called from $element[$slot_id] = self::buildSlotForm($form_state, $slot_id, $slot, $configuration[$slot_id] ?? [], $options);
  • Extract protected static function buildSourceForm(FormStateInterface $form_state, string $slot_id, array $definition, array $configuration, int $delta, string $wrapper_id): array called from $element['sources'][$delta] = self::buildSourceForm($form_state, $slot_id, $definition, $source_configuration, $delta, $wrapper_id);

TODO other:

  • Rename form elements?
  • Remove SourcePluginManager::buildPluginConfiguration()
πŸ‡«πŸ‡·France pdureau Paris

Merged. Future tasks will be done in dedicated issues.

πŸ‡«πŸ‡·France pdureau Paris

On the component template, I would keep only the logo stuff as understood from https://designsystem.digital.gov/components/header/ :

  • Slots: image
  • Props: path (url), text (string)

Non tested code, just a quick and humble proposal:

<div{{ attributes.addClass(['usa-logo']) }}>
  <em class="usa-logo__text">
  {% if path %}
    <a class="logo-img" href="{{ path }}" accesskey="1" title="{{ text }}" aria-label="Home">
  {% endif %}
  {{ image|default(text) }}
  {% if path %}
    </a>
  {% endif %}
</div>

{% if site_logo %}
{{ pattern('logo', {
'path': path(''),
'text': site_name,
'image': site_logo
}) }}
{% endif %}

Then on block--system-branding-block.html.twig (Non tested code):

{% if site_logo %}
  {{ pattern('logo', {
    'path': path('<front>'),
    'text': site_name,
    'image': site_logo
  }) }}
  {% endif %}
  {% if site_name %}
      <a href="{{ path('<front>') }}" accesskey="2" title="{{ 'Home'|t }}" aria-label="Home">
        {{ site_name }}
      </a>
  {% endif %}
  {% if site_slogan %}
    <h2 class="usa-font-lead">{{ site_slogan }}</h2>
  {% endif %}
  <button class="usa-menu-btn" type="button">{{ 'Menu'|t }}</button>
πŸ‡«πŸ‡·France pdureau Paris

I will test on my side

πŸ‡«πŸ‡·France pdureau Paris

So if I tried to use in layout builder, field group, or views and I assign a Drupal list field to this slot it won't work.

I guess you have this issue with collection_item because my proposal about alert looks compatible with site building.

Proposal: instead of doing this stuff with the filter() filter:
{% for key, meta_item in meta_items_list|filter((meta_item, key) => key|first != '#') -%}

We can do something like that:

{% set meta_items_list = (meta_items_list is iterable and (meta_items_list|keys[0] == 0)) ? meta_items_list : [meta_items_list] %} 
{% for meta_item in meta_items_list %}

So, when Twig will be ready, it will be easy to switch to the expected mechanism:

{% set meta_items_list = (meta_items_list is sequence) ? meta_items_list : [meta_items_list] %} 
{% for meta_item in meta_items_list %}
πŸ‡«πŸ‡·France pdureau Paris

This kind of code must be avoided because it is inspecting the structure of a render array in a slot:

{% for key, item in message|filter((item, key) => key|first != '#') %}

Slots content must not be inspected. They are opaque. Slots are not always render arrays.

This was found in two components alert and collection_item.

Proposed resolution for alert

This one is easy to fix because there is no need of such inspection if we follow the upstream documentation instead of managing the complexity of statys-message.html?tiwg inside the component template.

Let's keep the component template simple:

<div{{ attributes.addClass('usa-alert').setAttribute('role', role) }}>
  <div class="usa-alert__body">
    {% if heading %}
      <h4 class="usa-alert__heading">{{ heading }}</h4>
    {% endif %}
    {% if message %}
      <div class="usa-alert__text">
        {{ message }}
      </div>
    {% endif %}
  </div>
</div>

And move the loop in the presenter template:

  {% for type, messages in message_list %}
    {% for message in messages %}
      {{ pattern('alert', {
        'variant': type == 'status' ? 'info' : type,
        'message': messages,
        'attributes': attributes.addClass('margin-y-2')
      }) }}
    {% endfor %}
  {% endfor %}

By doing that, we are more compliant with upstream USWDS and we are avoiding 2 other risks: the use of merge filter and the alteration messages variable from inside a loop.

Proposed resolution for collection_item

This component is only called from search-result.html.twig which is sending a list of plain strings, so why inspecting the renderable?

Maybe because you want to be extra careful while waiting for ✨ Make the Twig loops safer Postponed , but I don't believe introducing this complexity will help.

I can obviously do this logic in the main twig file before calling the pattern but then I feel I can't use the pattern from the UI.

Hmmm.. Indeed we need to check that. Can you tell me more?

πŸ‡«πŸ‡·France pdureau Paris

In https://git.drupalcode.org/project/ui_suite_uswds/-/blob/3.0.x/templates... we have both single_message and singe_message:

       {% for single_message in messages %}
          <li class="usa-alert__text">{{ singe_message }}</li>
        {% endfor %}
πŸ‡«πŸ‡·France pdureau Paris

We also have this weird behaviour with empty markup:

  $variables['test'] =  [
    "#markup" => "",
  ];

{{ test ? "true" : "false" }}

Result:
true

Maybe we have to create an issue for Drupal Core.

πŸ‡«πŸ‡·France pdureau Paris

After analysis, this hook is already doing the job:

function ui_suite_dsfr_preprocess_page(&$variables) {
  $variables['container'] = theme_get_setting('container');

  $config = \Drupal::config('system.site');
  $variables['site_name'] = $config->get('name');
  $variables['site_slogan'] = $config->get('slogan');
}
πŸ‡«πŸ‡·France pdureau Paris

no news so merged

πŸ‡«πŸ‡·France pdureau Paris

Just a question, why not using UrlHelper::setAllowedProtocols()?

Maybe because mailto protocol doesn't work with UrlHelper which expects :/ instead of just :

I will merge

πŸ‡«πŸ‡·France pdureau Paris

Some thoughts...

1. Do we still do a separate component considering πŸ“Œ [beta6] ⚠️ Merge footer_top & footer_menu into footer component? Postponed ?

2. It will not be easy to convert main & sub slots to "links" props because of the images.

3. So, instead or aside of footer_partners component, we may need a new partner_link component with one url prop and one "image" slot:

{{ set attributes = url ? attributes.setAttribute("href", url) : attributes }}
<a{{ attributes.addClass("fr-footer__partners-link") }}>
  {{ image|add_class(""fr-footer__logo")|set_attribute("style", "height: 5.625rem")
</a>
πŸ‡«πŸ‡·France pdureau Paris

We put this issue on hold because the DSFR maintainers have yet to release a fix that will allow developers to define a list of (sub) partners without having to define a main partner (it shouldn't be long now)

3 month later, it seems it is not done yet upstream: https://www.systeme-de-design.gouv.fr/elements-d-interface/composants/pi...

Logo du partenaire principal, ferrΓ© Γ  gauche - obligatoire
Logos des partenaires additionnels, ferrΓ©s Γ  droite - optionnels.

πŸ‡«πŸ‡·France pdureau Paris

Let's wait the work is merged on USWDS before borrowing.

πŸ‡«πŸ‡·France pdureau Paris

The current mindset is that we want to be super defined about what is accepted in certain places like ctas where we don't necessarily want just anything passed there.

If you want to be superdefined about what is accepted in certain places, props are better than slots.

However, I would suggest to learn to "let go" while designing a design system. Components are meant to be shared and used with different business constraint and context. To achieve this, we need to both:

  • Make the components flexible
  • Make the components forward compatible

And slots are perfect for that. One day, instead of the super defined CTAs, your card may have something else. Those CTAs is not what make this component a card.

Also, in a Drupal context, slots have the advantage of being able to accept any renderable (rendered entities, views displays, forms...), so they are easy to use while site building from admin pages.

Anyway, if you keep them as props, {"type":"array"} and/or {"type":"array"} are not enough.

I wouldn't mind being pointed in the direction of some examples and articles to see what best practice is for those items.

Because this subject is still new in Drupal community, those articles are easier to find in other communities (like Vue.js, for example).

Also, witnessing the struggle of ReactJS with "prop drilling" teach us about the advantages of a proper slots system (which React lacks):

πŸ‡«πŸ‡·France pdureau Paris

Indeed, tested today, only the issues about table is remaining, and this one can be postponed.

πŸ‡«πŸ‡·France pdureau Paris

Monthly update: https://git.drupalcode.org/project/ui_suite_uswds/-/commit/5badac4cb9c97...

Only one remaining issue:
Unable to render component "ui_suite_uswds:table". A render array or a scalar is expected for the slot "header"

Let's check what UI Suite Bootstrap is planning to do.

πŸ‡«πŸ‡·France pdureau Paris

Sorry, in my previous message, i wrote attributes.setattribute() instead of attributes.setAttribute()
I forgot to tell you my proposal was untested code.

πŸ‡«πŸ‡·France pdureau Paris

Before doing the change, let's consider why we may wan to keep those components separated: page.html.twig
When we hardcode the call to a component from this template, we make its slots "configurable", because these are the regions where we put the blocks, but not its props. So menus will not be available anymore.

there is a solution cooking on UI SUite USWDS theme:

On theme settings:

  protected function menuSettingsForm($title, $value) {
    $all_menus = $this->entityTypeManager->getStorage('menu')->loadMultiple();
    $menus = [
      "" => "(None)",
    ];
    foreach ($all_menus as $id => $menu) {
      $menus[$id] = $menu->label();
    }
    asort($menus);
    return [
      '#type' => 'select',
      '#title' => $title,
      '#options' => $menus,
      '#default_value' => $value,
    ];
  }

it looks like the links settings type, but simpler.

On page preprocess:

protected function addRawMenu(array $variables, string $variable, string $menu_id): array {
$parameters = new MenuTreeParameters();
// Empty parameters because not configurable.
$tree = $this->menuLinkTree->load($menu_id, $parameters);
$manipulators = [
['callable' => 'menu.default_tree_manipulators:checkAccess'],
['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
];
$tree = $this->menuLinkTree->transform($tree, $manipulators);
$tree = $this->menuLinkTree->build($tree);
if (!isset($tree["#items"])) {
return $variables;
}
if (isset($tree["#cache"])) {
$variables["#cache"] = array_merge($variables["#cache"] ?? [], $tree["#cache"]);
}
$variables[$variable] = LinksSettingType::normalize($tree["#items"]);
return $variables;
}

once again, ike the links settings type, but simple.

πŸ‡«πŸ‡·France pdureau Paris

Ok, so let's wait for the module dedicated to icon management

πŸ‡«πŸ‡·France pdureau Paris

pdureau β†’ created an issue.

πŸ‡«πŸ‡·France pdureau Paris

i keep on my side, and will do a new conversion test when UI Patterns 2.x will hit a development milestone

πŸ‡«πŸ‡·France pdureau Paris

language_selector

language_label is not defined

template is printing item.label instead of the expected item.title

item.langcode must be item.attributes.lang.

So, instead of :
<span lang="{{ item.langcode }}" xml:lang="{{ item.langcode }}">{{ item.label }}</span>

You can do:

{% set attributes = attributes.lang ? attributes.setattribute("xml:lang",  attributes.lang ) : attributes %}
<span {{ attributes }}>{{ item.label }}</span>
πŸ‡«πŸ‡·France pdureau Paris

Thomas, is it what you are looking for ? ✨ Apply styles to Layout Builder layout regions Fixed

πŸ‡«πŸ‡·France pdureau Paris

Links type is now available.

πŸ‡«πŸ‡·France pdureau Paris

Thomas, do you believe your need can fit in https://www.drupal.org/project/ui_icons/issues/3349021 🌱 [Meta] Icon management Active ?

πŸ‡«πŸ‡·France pdureau Paris

Nothing happened here. Let's close it to clear the queue for the first release candidate.

Production build https://api.contrib.social 0.61.6-2-g546bc20