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

Merge Requests

More

Recent comments

🇫🇷France pdureau Paris

Nice to see so much great people here (David, Jean, Mike, Wim).

It would be nice to add a Icon API Form Element into Drupal Core. We didn't do it last December, in order to iterate in contrib space first. But it may be relevant now.

🇫🇷France pdureau Paris

Interesting. Also, I am proposing in this XB issue https://www.drupal.org/project/experience_builder/issues/3515074#comment... 🐛 References must not be required to guess props' JSON schema Active to move 2 UIP2 service to Core: they may be useful for XB too, they are inspired from a Python library, they don't depend on anything related to UI Patterns, and they implement pure JSON schema logic.

🇫🇷France pdureau Paris

I think the concerns would be reduced if [PP-1] Allow schema references in Single Directory Component prop schemas Postponed is done and core provided a handful of common definitions — such as for images.

Indeed, i have added a extensive comment [PP-1] Allow schema references in Single Directory Component prop schemas Postponed with the hope we will move fast on this subject. However, it may be a complicated task if we want to do things right.

Slightly tricky: if the ordering of the properties is different, it should also match. For example: not src/alt/width/height, but width/height/alt/src. I assume you agree those should all be treated as equivalent?

Of course, order of properties in a JSON object doesn't matter as far as I know. You can have a look on UI Patterns 2's Canonicalizer service: we are ksorting the properties before working on them.

The compatible subset example … is an interesting observation 😅 I hadn't thought of that one for sure! I want to think that through some more, but purely logically speaking, it's hard to refute.

You can have a look on UI Patterns 2's CompatibilityChecker service. We have happily used this for one year for similar needs.

🇫🇷France pdureau Paris

Just a little up-to-date summary of the 2 existing JSON Schema stream wrapper in contrib.

Experience Builder

  Drupal\experience_builder\JsonSchemaDefinitionsStreamwrapper:
    public: true
    tags:
      - { name: stream_wrapper, scheme: json-schema-definitions }

Example: json-schema-definitions://foo.module/bar

JsonSchemaDefinitionsStreamwrapper service is:

  1. Extracting the extension and the JSON schema $def ID from the URI. Example: foo module as extension and bar $def
  2. Load the schema.json file from the extension path. Example: /modules/contrib/foo/schema.json
  3. Return the extracted $def

Pro: is generic, any module can provide JSON schema definition without any dependency to Experience builder outside this stream wrapper.
Con: only static declaration

UI Patterns 2

  ui_patterns.schema_stream_wrapper:
    class: Drupal\ui_patterns\SchemaManager\StreamWrapper
    arguments:
      - "@plugin.manager.ui_patterns_prop_type"
    tags:
      - { name: stream_wrapper, scheme: ui-patterns }

Example: ui-patterns://bar

Streamwrapper service is:

  1. Extracting the . Example: foo module as extension and bar $def
  2. Load theschema.json file from the extension path
  3. Return the extracted $def

Pro: a dependency to plugin.manager.ui_patterns_prop_type service
Con: $defs are shipped alongside some niceties (type converter, data normalizer...) as a cohesive

Proposal

We can go the Experience Builder way because it is easier to ask UI Patterns 2 maintainers to add and maintain a schema.json file in their module instead of introducing a new plugin type to Core.

Also, this may be the opportunity to:

  • simplify a bit the URI scheme (previous proposal was $ref: module://experience_builder/image but we can propose another schema if "module" or "theme" are too generic) because this kind of URL will be typed a lot by component authors, with a lot of chance of mistakes
  • considerate Mateu's advice to use URI anchors to target the $defs ($ref: #) to leverage existing JSON schema mechanisms instead of extracting manually like JsonSchemaDefinitionsStreamwrapper is actually doing (however, I would advise to explore $anchor keyword instead of the propsoed JSON pointer

So, if we mix both, we can have something like that: $ref: module://experience_builder#image

This may not be enough

However, this proposal is not solving another issue reported by @finnsky : [2.0.0-beta5] Use HTTP URL isntead of a stream wrapper in JSON reference Closed: duplicate

We are becoming more and more generic, but it is still a Drupal only proposal (module and theme scheme, schema.json convention...).

What can we do to make those references working outside of Drupal (for example: in storybook)?

🇫🇷France pdureau Paris

Thanks you so much Kensae, your solution is very pragmatic, targeting exactly where the issue here in SDC prop conversion.

However, we are currently facing a similar issue in UI Example module: Support Single directory components Active and I would address to address both:

  • in a more generic way, because the root cause is the wrong sue of render children by SDC
  • in a similar way in both module

So, I let this in review state for now.

🇫🇷France pdureau Paris

Hi @nicxvan, you have open a MR, can you assign the issue to you so people will know you are working on it?

🇫🇷France pdureau Paris

From Enrique Lacoma:

Overriding components in a module, in the doc https://www.drupal.org/docs/develop/theming-drupal/using-single-director... says you can't override a component in a module, but I was able to do it, is the documentation correct or doing the override inside a module can create any issue? thanks

🇫🇷France pdureau Paris

So, we have agreed on using meta:enum and leveraging this information and the related translations (from locale module API) is up to the display building tools like UI Patterns 2 and Experience Builder.

Also, adding meta:enum to the documentation will be done in this issue 🌱 Clarify SDC documentation by toning down Twig blocks promotion Active .

So, what can we do in Core?

  • Add a mention of meta:enum in ComponentMetadata? Ans some light logic?
  • Add meta:enum to some of our test components
  • Add a specific test about meta:enum? which one?
🇫🇷France pdureau Paris

Review OK

But PHPCS fix to do.

I will create the drupal_cms_olivero issue, because the real problem is on their side.

🇫🇷France pdureau Paris

Suggestion from @mlncn about replaces property:

looking to improve the documentation slightly, the general advice should be to copy the entire component subdirectory into your themes components directory and add the replaces line. It's not like it can inherit anything it's a true replacement, yes?

Indeed, the replacement component is still a full component with its Twig template and its YAML definition.

🇫🇷France pdureau Paris

The projects might not be using them directly, but there are dozens/hundreds of preprocess hooks running on those projects via core and contrib modules.

Of course.

🇫🇷France pdureau Paris

but everything else in #99 I'm 100% agreed

@catch in #106 ? ;)

I don't fully understand #type => component or how to entirely get rid of preprocess via SDCs yet

In UI Suite community (not only the contributors in drupal.org, but also the agencies doing projects I am interacting with), we are not using preprocess hooks anymore, for many years, except for forms. I don't really know why it is a less an issue for us. Maybe because we fully switched to a design system + display builder paradigm. It is worth investigating and writing that down.

Adding three concrete active issues that could use reviews/help.

Thanks you. I will have a look after Atlanta.

I'm more likely to implement a twig extension instead of a preprocess function nowadays.

Twig extensions have a high risk of being harmful:

  • Written in PHP, they may hide some business logic specific to a specific project, or calls to CMS API. A SDC component is just a "dumb" piece of UI logic receiving already resolved data.
  • Written in PHP, they may introduce PHP in a Drupal theme (which we cant to avoid, a theme must be a front dev friendly place) or a dependency to module.
  • Instead of fixing the root cause (why this data is not OK to be injected in my template? We are moving the problematic mechanism elsewhere
🇫🇷France pdureau Paris

I wonder where this fits now that we have SDC, and Experience Builder is in early development?

We still need to work on this because:

  • SDC is addressing only UI components, an important part of a modern Render API but not everything
  • Experience Builder is a display builder, so an higher level tool. We need to address the lower levels

Nearly 5 years after my previous comment, here we are.

Current status

Let's not forget the Render API is great:

  • Declarative: Easy to type. (de)Serializable if clean.
  • Easy nesting: The Virtual DOM of Drupal. We are building a tree.
  • Data bubbling: Declare locally, impact globally
  • Asset libraries management: Our beloved libraries.yml
  • Clever caching: Context, tags, keys…

❤️ Building display by assembling configurable plugins returning renderables.

However, the renderables themselves are an issue.

They are too many of them and they are of 3 kinds:

  • 35 render elements in Core:
    $ grep -hoEr "#type' => '(\S+)'" core/ --exclude-dir tests | sort | uniq -c | sort -nr
    138 #type' => 'details'
      94 #type' => 'container'
      93 #type' => 'link'
      68 #type' => 'table'
      47 #type' => 'actions'
      36 #type' => 'inline_template'
      27 #type' => 'fieldset'
      23 #type' => 'html_tag'
      21 #type' => 'status_messages'
      14 #type' => 'pager'
  • 99 theme hooks in Core:
    $ grep -hoEr "#theme' => '(\S+)'" core/ --exclude-dir tests | grep -v "__" | sort | uniq -c | sort -nr
    84 #theme' => 'item_list'
    21 #theme' => 'username'
    16 #theme' => 'image'
      7 #theme' => 'status_messages'
      7 #theme' => 'image_style'
      6 #theme' => 'table'
      6 #theme' => 'links'
      5 #theme' => 'update_version'
      5 #theme' => 'indentation'
      5 #theme' => 'file_upload_help'
      …
    
  • 2 special ones:
       1016 '#markup'
           85 '#plain_text'
    

😱 135 renderables to learn/use? This is the issue.

What can we remove?

We have SDC for UI components, so:

  • #type => component can replace a lot of hook_theme, the one describing UI components: breadcrumb, progress_bar, links...
  • We can also replace the 34 render elements which are wrapper around hook_theme (because what they do, adding asset libraries, variable typing... is already done by SDC) we removed : table, status_messages, pager...

The hook_theme not suitable for for conversion to SDC are often annoying wrappers we can get rid of anyway : block, node, view, field

The few remaining render elements can't be expressed as UI components:

  • some must be kept because they represent other design systems artefacts (icons...) or low level bricks (HTML elements, placeholders...)
  • some may (not found in my today look) be shortcut to Drupal callables returning renderables, it is better to explicitly call the callable (service, function, method..) instead

Deprecation of ThemeManager::render()

If all template file based renderable are managed by SDC and because SDC is not passing through the ThemeManager, we dont need the template loading feature of the ThemeManager and we get remove it to get roi of the old confusing stuff:

  • Theme wrappers: Confusing and useless. You can always use an upper level instead.
  • Templates suggestions: Not discoverable. Messy. Blur the business / view separation.
  • Preprocess hooks: Risky. Unfriendly. Blur the business / view separation. Unpredictable order of execution.

Conclusion

We can have an equally useful Render API with less than 10 renderables (not counting Form API):

  • Design systems artefacts:
    • #type => component: for UI components
    • #type => icon: for UI components
  • Lower level bricks:
    • #type => inline_template
    • #type => html_tag : for HTML elements
    • #type => link
    • #markup
    • #plain_text
    • #lazy_builder

    Keeping the beloved universal render properties: #attached, #cache... and adding some related to design systems: #styles, #tokens, #mode...

    And now we have an purely design/UI based Render API!

    If we do that, step by step, deprecating slowly, not breaking anything, the display building tools from Core and Contrib will have a simpler implementation and will have an easier time introducing new features.

    To play safe, we can also introduce a new Render API alongside the existing, using @ instead of # for example:

    • #type => component, #component= "foo:bar" @component => "foo:bar"
    • #type => icon@icon => []
    • #type => inline_template, #template => "foo"@template => "foo"
    • #type => html_tag, #tag => "foo"
      @element => "foo"</li>
          <li><code>#type =>link

      @link

    • #markup@markup</li>
    • #plain_text@plain_text
    • #lazy_builder@lazy_builder
🇫🇷France pdureau Paris

Hi Christian,

Thanks for this issue. I hope I am understand what you are trying to do achieve here. I not, I will move this to its own ticket.

They are 3 levels in Field API and 3 kinds of prop types:

Indeed, we use typed_data property only with scalar prop types nowadays:

 $ grep -r typed_data src/Plugin/UiPatterns/PropType
src/Plugin/UiPatterns/PropType/UrlPropType.php:  typed_data: ['uri']
src/Plugin/UiPatterns/PropType/StringPropType.php:  typed_data: ['string']
src/Plugin/UiPatterns/PropType/NumberPropType.php:  typed_data: ['decimal', 'float', 'integer']
src/Plugin/UiPatterns/PropType/EnumPropType.php:  typed_data: ['float', 'integer', 'string'],
src/Plugin/UiPatterns/PropType/BooleanPropType.php:  typed_data: ['boolean']

Let's focus on the 3 red crosses (❌) instead

So we have much to do to cover the full field API.

Every component prop type has a (JSON) schema, every prop type has a (Typed data) schema. They may be matchable.

Example: Icon field with an Icon prop

Icon field:

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition): array {
    $properties = [];
    $properties['target_id'] = DataDefinition::create('string')
      ->setLabel(new TranslatableMarkup('Icon ID'))
      ->setRequired(TRUE);
    return $properties;
  }

is converted to JSON schema:

type: object
properties:
  target_id:
    type: string

Icon prop type:

type: object
properties:
  pack_id:
    "$ref": ui-patterns://identifier
  icon_id:
    type: string
  settings:
    type: object
required:
- pack_id
- icon_id

Not easy to map because it seems icon field has a single property (target_id) for 2 prop types properties (pack_id and icon_id). Maybe with a separator. Let's check that with UI Icon team.

Example: LinkItem field to Links prop

LinkItem field type :

   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
        $properties['uri'] = DataDefinition::create('uri')->setLabel(new TranslatableMarkup('URI'));
        $properties['title'] = DataDefinition::create('string')->setLabel(new TranslatableMarkup('Link text'));
        $properties['options'] = MapDataDefinition::create()->setLabel(new TranslatableMarkup('Options'));
        return $properties;
    }

is converted to JSON schema:

type: object
properties:
  title:
    type: string
  uri:
    "$ref": ui-patterns://url
  options:
    type: object

Links prop type:

type: array
items:
  type: object
  properties:
    title:
      type: string
    url:
      "$ref": ui-patterns://url
    attributes:
      "$ref": ui-patterns://attributes
    link_attributes:
      "$ref": ui-patterns://attributes
    below:
      type: array
      items:
        type: object

Not bad :) But url and uri property names don't match, so we can have 2 strategies:

  • We don't care about the property names ("title", "uri", "options"), we care only about the type and we let the builder do the mapping.
  • Can we use a PropTypeAdapter plugin transforming uri and url for that? They were not made for this kind of flow, but it may work.

Example: Multivalued StringItem field to List prop

StringItem field:

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['value'] = DataDefinition::create('string')
      ->setLabel(new TranslatableMarkup('Text value'))
      ->setSetting('case_sensitive', $field_definition->getSetting('case_sensitive'))
      ->setRequired(TRUE);
    return $properties;
  }

Is converted to this JSON schema:

type: object
properties:
  value:
    type: string

So this when multivalued:

type: array
items:
  type: object
  properties:
    value:
      type: string

List prop type:

type: array
items:
    type: ['string', 'number', 'integer']

It doesn't seem obvious at first, but it may be good match if we flat the single "value" field property.

Conclusion

None of my examples were a perfect match. But each of them is showing mechanism worth to study. There is something smart to do here.

🇫🇷France pdureau Paris

One year later, I am not that sure this must be it's own component instead of just an addition to the footer component.

🇫🇷France pdureau Paris

Great. Can you also change the plugin IDs accordingly?

🇫🇷France pdureau Paris

The 2 field widgets have similar and confusing labels:

  • "Component slot source" ('A field widget for a "component" source.'): the exact same label as the prop type?
  • "UI Patterns slot source" ('A field widget for an UI Patterns source.'): what is the difference with previous?
🇫🇷France pdureau Paris

we could also renovates our conversion mechanism which is now very simple and not easily alterable...

Wow! That sounds very interesting.

- a new plugin type to apply an operation from one prop type to the same prop type. those plugins may be used in components prop forms and component slot forms. One would select a source first, and one would optionally add a sequence of those plugins.
- a new plugin type to declare a conversion from one prop type to another. the introduction of those plugins would be done with a renovation of the prop type conversion system (declaration and conversion paths computation)

I agree only the first kind must be available in the admin UI, but I guess it can be a single plugin type because:

  • they share the same interface
  • we will also need the first kind in the revamped prop type conversion system
🇫🇷France pdureau Paris

but i will give a try to map, every data type needs to have a chance 🤣

Thanks you so much. Can you do some performance tests? If they are more or less the same, let's avoid the introduction of a custom data type.

🇫🇷France pdureau Paris

1

I am happily surprised to see you have added $element['#variant']. I believe our 2 "special" component props (attributes and variant) needs their own (optional) render properties and I need to create the related ticket for #attributes.

2

$component_attributes['data-component-variant'] = $context['variant'];

That's surprising too but why not.

3

  variantDefinitions:
    default:
      label: Default
      description: My default variant

Why a complicated lower camel case property: variantDefinitions. I am already quite a few people struggling with libraryOverrides instead of the more straightforward and expected library. Can we have this instead:

  variants:
    default:
      label: Default
      description: My default variant

4

In my-cta.component.yml, variantDefinitions is inside the prop property:

props:
  type: object
  required: []
  properties: {}
  variantDefinitions: {}

It is not compliant with JSON schema standard. Can we move it to the component definition root?

5

We use label but:

  • component is using name
  • slots are using title
  • props are using title

Instead of introducing a third way of declaring the same information, can we copy the current slot structure?

6

Is it not too late?

    if (empty($variant) || isset($context['variant'])) {
      $template .= sprintf('{%% embed \'%s\' %%}', $id);
    }
    else {
      $template .= sprintf('{%% embed \'%s\' with { variant: \'%s\'} %%}', $id, $variant);
    }

Why tweaking the dynamically generate template instead of moving the variant to the props a bit earlier?

Non tested naive code snippet:

    if (isset($element["#variant"] && other_condition_we_may_add)) {
      $element["#props"]['variant'] = $element["#variant"];
    }
🇫🇷France pdureau Paris

Sorry to disturb that late, but do we really need a custom data type (Source) storing the data in JSON?

There are already data type in Drupal Core for such data structure which are (theoretically, we need to check) more performant for decoding/encoding nested PHP arrays, like https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData...

XB is also using a custom data type based on JSON, but I guess they send the exact data as HTTP Response Body, without the need of unserialize it to PHP arrays

What do you think about that?

🇫🇷France pdureau Paris

There are multiple nice usecases.
for example, to invert a boolean value, to filter an output, to trim a string... etc etc

Indeed, and we need to scope this. I would like to introduce a restriction to avoid rewriting a full data processing framework inside UI Patterns 2 and maange unexpected side effects: we transform from the same prop type to the same prop type.

So:

  • from attributes to attributes: any attribute source can be a filter, the attribuets are merged at each steps with something like AttributeHelper::AmergeCollections
  • from string to string: trim, upper, replace...
  • from boolean to boolean: invert...

Is it a new plugin type? I was initially kind of investigate TypedDataFilter plugins from https://www.drupal.org/project/typed_data
What else can we study from Drupal contrib?

🇫🇷France pdureau Paris

It really depends on how XB and equivalents implement the UI.

Just a side-note: Let's not implement this fetaure only to please some specific display builders. Let's do it the better way in a SDC point of view, and the display building tools will be able to add features upon it.

The key suggested seems somewhat weak for this action.

Maybe we need different keys:

  • suggested: for the DaisyUI card example.
  • enforced: for the Carousel example

So, let's see how a display building tool can handle those:

It also might be worth exploring whether we can limit an SDC to only appear in a specific slot of a specific SDC. A use case of this is that I don't want my end user to be able to place an accordion_item in the main content area. It only works when placed inside of accordion_group

Interesting path to explore indeed:

  • Is it always the reverse of enforced? So no need to add a new keyword?
  • If we need a new keyword, will it be also in the parent's slot definition ? Or in the child definition?

Let's be careful with any restriction we would add. For example, if the design system is expanded by a distinct team with new components, and one of the also want to use accordion_item in of those slots, we need to allow that.

🇫🇷France pdureau Paris

Thanks a lot Mike for this issue which will make a lot of people happy.

Instead of " are allowed" i would say "are suggested" because it would be a mistake for display building tools to strictly enforce this:

  • sometimes business needs are surprising from a design system point of view and business always win. So, if a card component suggests
  • sometimes a display builder doesn't know which component is a renderable. For example, placing a view inside a component slot. The view is build with another component, but the component is not know yet.

So, it is up to the display building too to decide how strict it will be considering this suggestion, but from a SDC point of view it is only a suggestion.

So, an early proposal with an example similar to yours, Bootstrap's accordion:

name: Accordion
description: "Render content in a box that expands and collapses vertically."
group: Accordion
slots:
  content:
    title: Content
    description: "Accordion items."
    suggested: [accordion_item]

Example with DaisyUI's Card:

name: Card
description: "Cards are used to group and display content in a way that is easily readable."
group: "Data display"
slots:
  image:
    title: Image
  title:
    title: Title
  text:
    title: Text
  actions:
    title: Actions
   suggested: [badge, buttons]

What do you think?

🇫🇷France pdureau Paris

The point of all this: thinking through what it'd mean to go much further in allowing SDC developers to specify the authoring experience for Content Creators.

Hi Wim, it has been a long time you haven't heard of my warnings so here I am again 😉

The mission of SDC developers (as component authors) is to provide the best implementation for UI components, they don't have to think about the business/applicative/CMS side, so they don't have to specify the content editor experience. Once a component author starts to think about the CMS, he is in risk of lowering the quality of its UI Component.

It is not only a theoretical stance, we are doing this with UI Patterns since 2017 and it works well. UI Patterns 2.x is the proof it still work (and work even better IMHO) with SDC: we are able to inject data to slots & props from many sources.

Content editors are not the only users of a component. An UI component has to be as usable in other contexts (non editorial, unexpected constraints, data retrieval from API...).

🇫🇷France pdureau Paris

Thanks you.

2 possible fix:

🇫🇷France pdureau Paris
  • getChildPluginDefinitions >> getItems
  • getSettingsSourceWithChildPlugin >> getMinimalSetting
  • getChildPluginId >>getSelectedItem
interface SourceWithItemsInterface {

  /**
   * Get items.
   *
   * @return array
   *   Associative array of items definitions with ID as the key.
   */
  public function getItems(): array;

  /**
   * Get the minimal settings for an item.
   *
   * @param string $item_id
   *   An item ID.
   *
   * @return array
   *   A source setting.
   */
  public function getMinimalSetting(string $item_id): array;

  /**
   * Get the currently select item.
   *
   * @return string|null
   *   The item ID.
   */
  public function getSelectedItem(): ?string;

Is it better?

Also:

  • Do we also need a getItemLabel(string $item_id): string ?
  • it may be good to move ::getMinimalSetting() to SourceInterface and to implemtn it in SourcePluginBase as a proxy to PluginSettingsInterface::defaultSettings(). What do you think?
🇫🇷France pdureau Paris

getMinimalConfiguration does not need any parameter, unless it is static, which could be a nice idea after all.

Oops, a copy paste mistake, I fix it in my comment

🇫🇷France pdureau Paris

Hi @freelock,

But now trying to make an SDC, we've created a menu_menu SDC and a menu_item SDC, and placed the menu_menu SDC using UI Patterns, as a block. So far so good, we have our menu.

Nice, an user of UI Patterns 👍 I hope you enjoy using it.

but it feels a bit nasty and introduces other dependencies (Twig Tweak).

Twig Tweak is nasty :) Its philosophy is the reverse of SDC one.

I would much rather simply call the child SDC (menu_item) from within the parent, but I don't have any access to the menu_item_content entity!

Are you using a slot in menu_menu to inject menu_item components and a slot in menu_item to inject menu_menu components?

Maybe not because it seems you are manipulating a prop structure here:

The items do have a "below" array of child items...

Can you share with us the definition and the template of the 2 components?

🇫🇷France pdureau Paris

I am not sure about the naming...

Proposal inspired from [ObjectWithPluginCollectionInterface](https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Plugin%21...) but not identical because we have a single collection and not many collections:

interface SourceWithCollectionInterface {

  /**
   * Get collection of items.
   *
   * @return array
   *   Array of items definitions.
   */
  public function getCollection(): array;

  /**
   * Get the minimal source configuration for an item.
   *
   * @param string $item_id
   *   An item ID.
   *
   * @return array
   *   A source configuration.
   */
  public function getSourceConfiguration(string $item_id): array;

  /**
   * Get the currently select item from the collection.
   *
   * @return string|null
   *   The item ID.
   */
  public function getSelectedItem(): ?string;

Do we also need a getItemLabel(string $item_id): string ?

🇫🇷France pdureau Paris

why is it called "Media: background image", there is nothing specific to background here, it depends of the usage in the component template, no?

🇫🇷France pdureau Paris

Hi Goz,

When we will move to entities, views & formatters for the component library, we will be able to do a formatter for that. See 📌 [2.0.0-alpha3] Use Drupal entities & plugins for the Library pages Active (expected late summer 2025)

Until then, is it possible to do it in the template? Something with json_encode() with PRE and/or CODE HTML elements?

I am not pushing this solution, I am just asking.

🇫🇷France pdureau Paris

Do I need to somehow define what the keys are in the associative array?

I guess you have to. You need to describe the data you expect, so someone (or something) using your component knows what to inject here.

So, you had this:

    options:
      type: array
      items:
        type: object

Processed like that in your template:

{% for option in options %}
      {% if option.type == 'optgroup' %}
        <optgroup label="{{ option.label }}">
          {% for sub_option in option.options %}
            <option value="{{ sub_option.value }}">{{ sub_option.label }}</option>
          {% endfor %}
        </optgroup>
      {% elseif option.type == 'option' %}
        <option value="{{ option.value }}">{{ option.label }}</option>
      {% endif %}
    {% endfor %}

So, your schema may be (non tested proposal):

    options:
      type: array
      items:
        type: object
       properties:
         label: {type: string}
         value: {type: string}
        options: {type: object}

I know there is still an undefined options object but it is already usable like that.

🇫🇷France pdureau Paris

Thanks so much. I will have a look.

🇫🇷France pdureau Paris

Data primitives mapping between the 3 formats we are manipulating:

Maybe adding such a table will be useful in the SDC documentation.

🇫🇷France pdureau Paris

Hi Liam,

I am a little worried about this sentence:

I think that I should replace type: object with type: array (because) the PHP type is an associative array.

AFAIK, here is the data primitives mapping between the 3 formats we are manipulating:

So, an associative array seems to be type: object.

🇫🇷France pdureau Paris

We may do a documentation session with @mherchel during DrupalCon Atlanta

I am very excited about that. There are many sides of SDC documentation which need to be clarified or completed after 2 years of practice.

🇫🇷France pdureau Paris

I beleive we need to get rid of those PHP Namespace properties instead of extending their usage.

So, in my humble opinion, no need to add 100 lines of code for this, because:

  • They were a mistake from the early days of SDC.
  • They are a "drupalism", non-compliant with JSON Schema standard.
  • SDC is doing weird stiff to skip JSON schema validation for them.
  • They create a coupling between UI components and CMS application.

So, it may be better to create an issue to raise a warning each time someone is using those.

🇫🇷France pdureau Paris

I believe we don't need to specify both prop_types: ['string', 'url'].

We target the more specific, so URL, and it will also be available for the more generic, string

Can you try?

🇫🇷France pdureau Paris

Which PHP object are you sending as prop?

🇫🇷France pdureau Paris

Hi Kristen,

Since October:

  • I am getting in touch with sdc_styleguide maintainer from time to time, adopting the shared story format seems still planned but no date shared yet.
  • UI Patterns 2.0.0 was released in February with great traction, being the first stable module using the story format
  • steepbase module has also adopted this format in January, and I hope more will join this year.
🇫🇷France pdureau Paris

this is not JSON schema compliant:

      allowed_bundles:
        image: 'field_media_image'
      image_style: 'large'

We need to find a way of expressing this data in a JSON schema way.

🇫🇷France pdureau Paris

Let's try to move fast on this issue.

So, we agreed each field item must be a single "object" and if we want a collection of them we configure the field as multivalued.

What would be this object ? I am proposing it to have at least 2 field properties:

public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
  $properties['source_id'] = DataDefinition::create('string')
    ->setLabel(new TranslatableMarkup('ID'));
  $properties['source'] = MapDataDefinition::create()
    ->setLabel(new TranslatableMarkup('Data'));
  return $properties;
}

Mapping exactly the current UI Patterns 2 form data structure.

For example, a component source:

source_id: component
source:
  component:
    component_id: 'ui_suite_bootstrap:accordion_item'
    variant_id: null
    slots: []
    props: {  }

A block:

source_id: block
source:
  plugin_id: announce_block

A block with configuration:

source_id: block
source:
  plugin_id: 'views_block:content_recent-block_1'
  'views_block:content_recent-block_1':
    id: 'views_block:content_recent-block_1'
    label: 'Recent content'
    label_display: ''
    provider: views
    views_label: 'Recent'
    items_per_page: '5'

An entity field:

source_id: entity_field
source:
  derivable_context: 'field:node:article:status'
  'field:node:article:vid':
    value:
      add_more_button: ''
  'field:node:article:status':
    value:
      add_more_button: ''

Fabien, Christian, Mikael, are you OK with those 2 first field properties? Which other properties do you need?

These processors can be configured during the mapping process. This can also help to reduce the mapping if a config field exists.

Interesting but it looks like a distinct issue, no?

🇫🇷France pdureau Paris

Thinking aloud: Is it limited to slot sources ? Why not opening this alter to prop sources?

🇫🇷France pdureau Paris

Discussed with Mikael. We are not altering the source but the source value:

  • hook_component_slot_value_alter (component_slot_value)
  • hook_sdc_slot_value_alter (sdc_slot_value)
  • hook_ui_patterns_slot_value_alter (ui_patterns_slot_value)
  • hook_slot_value_alter (slot_value) if the previous ones are too long

But I still prefer the "slot_source" one ;)

🇫🇷France pdureau Paris

Naming is hard :)

We are allowing the alteration of a slot source value:

  protected function buildSlot(array $build, string $slot_id, array $definition, array $configuration, array $contexts): array {
   ...
    foreach ($configuration["sources"] as $source_configuration) {
      ...
      $source_value = $source->getValue($slot_prop_type) ?? [];
      $this->moduleHandler->alter('component_built', $source_value, $source_configuration);
  }

So, instead of hook_component_built_alter (component_built), we can propose hook_component_slot_source_alter (component_slot_source)? Is it too long?

🇫🇷France pdureau Paris

[attributes] string value found .."

This issue is well known.

Your attributes value is going through the SDC validator, which is normal when using UI Patterns 2.

But the validator receive a PHP object which is casted as a string and raise an error because it doesn't expects a string, and this is not normal because UI Patterns 2 is preventing that.

What is happening if you skip header.twig and call directly your component from region-header.html.twig ?

Also, it is better to use "with_context: false" with {% embed 'my_components:header' ...

🇫🇷France pdureau Paris

Duplicate: [2.0.1] StoryPluginManager::getComponentStories() must return sorted results Active

So, moved to 2.0.1.

No need to implement the full CategorizingPluginManagerInterface because stories are not goruped

Some people also want weighted results, but this will be another task.

🇫🇷France pdureau Paris
I think everybody have differnt use cases in mind. We should merge our ideas.

Indeed. Different use cases doesn't necessary mean different implemntations.

🇫🇷France pdureau Paris

ok i will work on clarifying the scope

🇫🇷France pdureau Paris

This is an important error to raise in my humble opinion.

This prop schema doesn't tell enough about the prop and the expected data. It is not useful for the component users and for tools loading the component into Drupal, like UI Patterns 2 or Experience Builder .

What kind of data is expected in each object item? Which properties ?

Do I need to give a more specific definition, such a the class or an interface?

No PHP classes. Let's stick with legit JSON schema ;)

Do you know what to add?

🇫🇷France pdureau Paris

From Liam in 🐛 The "empty is not needed" test is not correct Active

This condition: {% if variable is not empty %}
raised the warning "The exact same as just testing the variable, empty is not needed."

But if I switched it to: {% if variable %}
then the condition would fail if variable was zero.

Let's check if empty is really same as just testing the variable before setting our "liberal" list of warnings.

🇫🇷France pdureau Paris

Hi Liam,

Thanks for your feedback. Let's discuss about this in the more generic issue: 📌 Adopt a more liberal approach for TwigValidatorRuleTestExpr Active

🇫🇷France pdureau Paris

It is a very nice SDC component! I love the facts you are not using Twig blocks fro your slot, iri-reference for URL prop, and both the definition and the template are very clean 👍

A few feedbacks however...

Little cleanups

Slots are not typed, so you need to remove "type" property:

slots:
  content:
    type: string
    description: Message content.

It seems you are using BEM methodology for the class naming, so this:

{%
  set classes = [
    'toolbar-message',
    'toolbar-message--type--' ~ type,
  ]
%}

Would be better written like:

{%
  set classes = [
    'toolbar-message',
    'toolbar-message--' ~ type,
  ]
%}

data-drupal-tooltip

Don't use a slot as an attribute value. A slot can have anything, including markup, and may break the browser DOM.

setAttribute('data-drupal-tooltip', content).

So, what can we do? The obvious options would be:

  • Make content a string prop instead of a slot, especially if only plain text is expected here
  • Create a new string prop for this attribute value

However, the best would be to remove those attributes from the template:

setAttribute('data-drupal-tooltip', content).setAttribute('data-drupal-tooltip-class', 'admin-toolbar__tooltip')

Because they tell us about the context how/where the component is used instead of being proper parts of the component.

I would inject them from outside (non tested proposal):

          '#type' => 'component',
          '#component' => 'navigation:message',
          '#props' => [
            'type' => 'warning',
            'url' => $url,
           'attributes' => new Attribute({
              'data-drupal-tooltip': content, # raw? processed? or something else?
              'data-drupal-tooltip-class': 'admin-toolbar__tooltip'
          })
          ],

Side note

I am recommending executing drush sdcv umami from https://www.drupal.org/project/sdc_devel/ to automatically get useful warning and errors.

There are still some rough edges (unclear messages, false positives...) but this tools is already usable.

🇫🇷France pdureau Paris

Hello Liam,

sdc_devel validator is working on the AST (abstract syntax tree) from the already parsed template, so sometimes 2 different template syntaxes are parsed as the same nodes and trigger the same warnings/errors.

However, it seems those expressions produces clearly different nodes:

@mogtofu33: What do you thing about that?

🇫🇷France pdureau Paris

@doxigo Thanks a lot for you in investigation. Can you tell us more about the root cause on Drupal Core side?

🇫🇷France pdureau Paris

In UIP2, I would say the prop type is string

Sure about that? Why not a specific prop type?

What would be the JSON schema of this prop type?.

🇫🇷France pdureau Paris

Hi Liam,

We are currently changing our mind and agreeing with you. That's why we have such a ticket.

🇫🇷France pdureau Paris

2.1.0 is becoming busy. I propose to move this to 2.2.0 with the other source plugins additions.

🇫🇷France pdureau Paris

Why not use #children as 3 lines above.

I still believe #children is an internal property of the render API and we are not supposed to use it.

Except 3 messy snippets, it is used by Drupal Core only in the Render API internals:

  • web/core/modules/views_ui/src/ViewEditForm.php
  • web/core/modules/layout_builder/src/Controller/ChooseSectionController.php
  • web/core/modules/system/src/Controller/DbUpdateController.php
  • web/core/lib/Drupal/Core/Render/Renderer.php (many times!)
  • web/core/includes/form.inc (many times!)
  • web/core/includes/theme.inc (a few times)
🇫🇷France pdureau Paris

Thats wrong. Nobody will understand it:).

You are talking about presenter templates, right? Nobody understand them already :) and they will disappear soon hopefully. that's why we are extra careful and future-proofing in SDC templates, but we care less in presenter templates.

Production build 0.71.5 2024