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

Merge Requests

More

Recent comments

🇫🇷France pdureau Paris

Thanks a lot for the detailed review.

Edit the Builder and add a token and views rows, note: I don't see the DB plugin '[View] Rows' but the UI patterns source 'Views rows'

I will have alook.

If Display builder is enabled on a view, and the view not saved, if click on the view on the Display builder > build display, this redirect to 'Page not found'
> Expected: If there is a link and the builder appear in 'Views builder', should be able to edit it

I will have a look.

When enabled display should we have the default views zone field set to not start empty?

It is already the case with this I believe:

  public function initInstanceIfMissing(): void {
     ...
    // Get the sources stored in config.
    $sources = $this->getSources();
    if (empty($sources)) {
      // Fallback to a fixture mimicking the standard view layout.
      $sources = DisplayBuilderHelpers::getFixtureDataFromExtension('display_builder_views');
    }
    $this->stateManager->create($instance_id, (string) $this->getDisplayBuilder()->id(), $sources, $contexts);
  }

But the fixture is added to the State Maanger instance only and you need to save the Display Builder to have it in config. It works the same for 📌 [1.0.0-beta1] Add proper page management Active and 📌 Implement WithDisplayBuilderInterface in Entity View Active . Do you want to save it in config directly?

🇫🇷France pdureau Paris

The draft spec seems to recommend .tokens or .tokens.json, so I assume ours would be .tokens.yml

If we go the DTCG way, I would recommend to support both YAML and JSON format.

Our plugin discovery will be YAML only, using the common Drupal\Core\Plugin\Discovery\YamlDiscovery:

  • So, filename will be: {provider}.tokens.yml
  • The data structure will strictly follow DTCG
  • That's neat because the first level is a mapping where the keys are our plugin ID:
shadow:
  "$type": shadow
  "$value": { .. }
font-size:
  "$type": dimension
  "$value": {...}

Because JSON is a subset of YAML (not exactly but it is enough for us), front developers can simply copy paste the content of .tokens.json or .tokens file provided by upstream inside the YAML and it works.

That would be my recommendation, but we can go further, and also do a multiple YAML discovery using decorators to support those 3 filenames but we :

  • {provider}.tokens.yml
  • {provider}.tokens.json (or {anything}.tokens.json?
  • {provider}.tokens (or {anything}.tokens?

We will need to discuss precedence of identical plugin IDs in those files.

🇫🇷France pdureau Paris

Time flies. I move to alpha4. We need 📌 Implement WithDisplayBuilderInterface in Entity View Active to be merged first anyway.

🇫🇷France pdureau Paris

Remove shoelace library from display builder config entity because it may not be necessary to have different config by Display Builder and because it breaks the previews in UI Patterns Library.

Moved to its own issue: 📌 Remove shoelace library from display builder config Active

🇫🇷France pdureau Paris

A few improvement proposals.

Add form:

  • Redirect to /admin/structure/display-builder after entity creation instead of showing a new create form
  • Default values: the same as display_builder.display_builder.default

Add & edit forms:

  • And add a checkbox to activate the related permission by role (like what Text Formats are doing)?
  • Remove shoelace library from display builder config entity because it may not be necessary to have different config by Display Builder and because it breaks the previews in UI Patterns Library.

Collection page (/admin/structure/display-builder)

  • Add a col with roles having the related permission in /admin/structure/display-builder (like /admin/config/content/formats is doing)
  • Remove machine name col
  • Reorder by adding a weight property and implementing DraggableListBuilder. Add a little explanation, like Text Format. (and update ConfigFormBuilder::buildDisplayBuilder())
🇫🇷France pdureau Paris

Also, if we try to improve our config schema...

From:

    display_builder:
      type: string
      label: "Used Display Builder"

To something like that:

    display_builder:
      type: string
      label: "Used Display Builder"
      constraints:
        ConfigExists:
          prefix: 'display_builder.display_builder.'

From:

    sources:
      type: ignore
      label: "Sources"

To something like that:

    sources:
      type: sequence
      label: 'Sources'
      sequence:
        type: ui_patterns_source.[id]
        label: 'Source'

extending DefaultLazyPluginCollection

Is it unlocking new cool Drupal feature like automatic casting ? automatic dependency calculation ? so we can reduce the complexity of our code

🇫🇷France pdureau Paris

FYI, in 📌 Align config storage properties & mechanisms Active is_display_builder_views will become views:style

🇫🇷France pdureau Paris

I take it back, i need to add little rule in ::initInstanceIfMissing()

🇫🇷France pdureau Paris

It looks good! Thanks a lot.

Some little proposals.

Definitions

Props:

  • type title is missing
  • content description is the same as the title, you can remove it

Templating

I am afraid {% if url is not empty %} is not catching when url value is missing. I would recommend the simpler {% if url %}

🇫🇷France pdureau Paris

Tested with Mikael, works OK.

However, we need to apply this drafty patch to make it works:

diff --git a/modules/display_builder_entity_view/src/Entity/DisplayBuilderEntityViewDisplay.php b/modules/display_builder_entity_view/src/Entity/DisplayBuilderEntityViewDisplay.php
index bb35966..4d731fb 100644
--- a/modules/display_builder_entity_view/src/Entity/DisplayBuilderEntityViewDisplay.php
+++ b/modules/display_builder_entity_view/src/Entity/DisplayBuilderEntityViewDisplay.php
@@ -238,7 +238,7 @@ class DisplayBuilderEntityViewDisplay extends LayoutBuilderEntityViewDisplay imp
     $this->stateManager->create($instance_id, (string) $this->getDisplayBuilder()->id(), [], $contexts);
     // Careful. StateManager::create() is not saving properly the contexts.
     // Let's do it again.
-    $this->stateManager->setContexts($instance_id, $contexts);
+    //$this->stateManager->setContexts($instance_id, $contexts);
   }
 
   /**
diff --git a/src/IslandPluginBase.php b/src/IslandPluginBase.php
index dd69177..f76e068 100644
--- a/src/IslandPluginBase.php
+++ b/src/IslandPluginBase.php
@@ -47,7 +47,7 @@ abstract class IslandPluginBase extends PluginBase implements IslandInterface {
   /**
    * The contexts for islands.
    */
-  protected array $contexts = [];
+  //protected array $contexts = [];
 
   /**
    * {@inheritdoc}
@@ -64,8 +64,8 @@ abstract class IslandPluginBase extends PluginBase implements IslandInterface {
   ) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
     $this->data = $configuration;
-    $this->contexts = $configuration['contexts'] ?? [];
-    unset($configuration['contexts']);
+   // $this->contexts = $configuration['contexts'] ?? [];
+   // unset($configuration['contexts']);
     $this->setConfiguration($configuration);
   }
 

It is related to 📌 Add the island configuration forms Active

🇫🇷France pdureau Paris

Remain work:

  • Regression, display not rendered. Looks like a context issue.
  • Remove DisplayBuilder::DISPLAY_BUILDER_CONFIG again
  • Update devel controller again
🇫🇷France pdureau Paris

Let's keep this issue and MR only for Views integration and the rest move there: 📌 Implement WithDisplayBuilderInterface in Entity View Active

🇫🇷France pdureau Paris

There are 3 kinds of Source plugins in UI Patterns 2:

  • "Widgets": simple form elements storing directly the data filled by the user. For example, a textfield for a string or a checkbox for a boolean.
  • Sources retrieving data from Drupal API: they can be context agnostic (ex: a menu for links) or context specific (ex: the title field for a string)
  • Context switchers: They don't retrieve data but they give access to other data sources. For example, the author fields from an article content.

We have already improved the UX of context switchers by renaming them like this: [Current context] ➜ [New context]: [Entity] ➜ [Field], [Field item] ➜ Referenced [Entity]...

Can we do something similar for widgets? Knowing those facts:

  • There is already a tag to know which sources are widgets or not
  • Normally, there is only one widget by prop type, but community can add more in their projects

Or, more specifically, we do something special when the default source (we know this one thanks to the PropType attribute) is a widget, so we are sure there is only one:

This default + widget source are:

  • AttributesPropType: attributes
  • BooleanPropType: checkbox
  • EnumListPropType: selects
  • EnumPropType, VariantPropType: select
  • EnumSetPropType: checkboxes
  • IdentifierPropType, StringPropType: textfield
  • >> not a widget
  • ListPropType: list_textarea
  • NumberPropType: number
  • >> not a widget
  • UrlPropType: url

We are already putting them at the top of the selector (don't we?), what can we do more? Some wording as proposed in the issue? Some hints?

For information, SDC Display doesn't have this issue because there is only 2 "sources" by prop type:

  • "Static": a widget with direct filling of data
  • "Dynamic": a contextualized data retrieval

What can we do to simplify our UX?

🇫🇷France pdureau Paris

Nearly done. It will be a biiiiig MR.

DONE.

  • Implement EntityWithDisplayBuilderInterface for View
  • Implement EntityWithDisplayBuilderInterface for Entity View
  • Implement EntityWithDisplayBuilderInterface (partially) for Devel
  • Same form for all (ConfigFormBuilder)
  • Remove
    StorageProperties::InstanceId->value</code</li>
      <li>Remove <code>DisplayBuilder::DISPLAY_BUILDER_CONFIG
  • Remove enabled in Entity View
  • Fix: "the render is made from the State Manager data instead of the data saved in config
  • Remove some "dead" Layout Builder logic in Entity View
  • Remove DisplayBuilderViewsManager

."

Remaining:

  • Fix regressions
  • Fix PHP Stan
  • Remove DisplayBuilderEntityViewDisplayStorage ?

Folllow-up issue:

  • Rename EntityWithDisplayBuilderInterface to WithDisplayBuilderInterface
  • Merge StorageProperties enum to static constant in ConfigFormBuilder?
🇫🇷France pdureau Paris

We will also be able to remove StorageProperties::InstanceId->value, because there will have no need of storing this to config now we have EntityWithDisplayBuilderInterface::getInstanceId()

🇫🇷France pdureau Paris

XB should integrate an icon picker similar to the UI Icons module.

Planned for Drupal 11.3: Add form element(s) for Icon API Active

It is one of the 5 main design-systems related issues planned for 11.3, with:

It is ambitious, but it is doable. Let's make Drupal the "first deisgn-system native CMS".

🇫🇷France pdureau Paris

We don't have this feature in Drupal pagination, the last page is already available with 'last' property.

Indeed, it is present in the data retrieved from Druapl API, but we are not forced to use it in our pager.html.twig (and views-mini-pager.html.twig?) presenter template(s), or in plugins with the Render API.

🇫🇷France pdureau Paris

We may need to port a yaml_encode Twig filter like https://symfony.com/doc/current/reference/twig_reference.html#yaml-encode

Where? 2 proposals:

  • in UI Patterns Library, where we already have a TwigExtension class.
  • in a more generic new Drupal contrib module

In both cases, that means the theme using the filter will need to declare the dependency to ui_patterns_library or the other module

🇫🇷France pdureau Paris

Waiting pipeline. Automerge will not be triggered. You can merge, it is OK

🇫🇷France pdureau Paris

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

🇫🇷France pdureau Paris

Now we have a pretty OK compoennt structure, let's check if it is usable by UI Patterns before merging.

Proposals:

  1. At the ViewRow level, add a new ViewRow plugin which is removing the view_fieldstheme wrapper
  2. At the ViewRow level, alter ViewRow::render() outputs to remove view_fields theme wrapper
  3. At the ViewStyle level, remove the view_fields theme wrapper from ViewRowsSource::getPropValue()
🇫🇷France pdureau Paris

@d34dman A lot of exciting work here: 🌱 Gradually replace Drupal's AJAX system with HTMX Active

🇫🇷France pdureau Paris

No update here but this issue is still working on 👍 We are still excited and we are still targeting Drupal 11.3

🇫🇷France pdureau Paris

Anybody can take this ticket :)

🇫🇷France pdureau Paris

Waiting for answer. Moved to alpha4.

🇫🇷France pdureau Paris

Moved to alpha4 because we need 📌 Align source contexts Active before

🇫🇷France pdureau Paris

CI pipeline running. Must be OK

🇫🇷France pdureau Paris

This is also the opportunity to fix a bug in Display Buidler View: the render is made from the State Manage data instead of the data saved in config.

🇫🇷France pdureau Paris

The expected config was not to display all the buttons or not, but only the "Reset" button:

display "Clear" button in history buttons (must be hidden by default)

🇫🇷France pdureau Paris

It seems DisplayBuilderEntityViewDisplay can also implements the upcoming EntityWithDisplayBuilderInterface :)

🇫🇷France pdureau Paris

This new ui_patterns_views_rows_as_fields configuration in the ViewRowsSource plugin is not necessary because:

  • there is already a system to force the fields at the View plugins level
  • "fields" here are related to the API and the configuration model, not necessary the returned renderable

It would be better to catch the returned renderable from some ViewRow plugins, and to tidy it. If we want a list of renderables:

  • the array must not be an associative array
  • the list must not be wrapped in a useless renderable from views module
🇫🇷France pdureau Paris

Last task remaining: better switch in ThemeRegistryAlter

🇫🇷France pdureau Paris

3 information:

  • Instance ID are used in HTML classes, so let's avoid dots.
  • "display_builder_" prefix is already added by State Manager's Storage service, no need to specify it here
  • Entity ID is enough as a scope, because all entity IDs are in the same namespace

So, proposal: {entity_type_id}--{entity_id}

🇫🇷France pdureau Paris

Also, this is the opportunity of adding config dependency management

🇫🇷France pdureau Paris

I take it because I work on similar topics in 📌 [1.0.0-beta1] Add proper page management Active

🇫🇷France pdureau Paris

Committed 6fba210 and pushed to 11.x. Thanks!

🇫🇷France pdureau Paris

By the way, some of the logic expected in Drupal\ui_patterns_overrides\Plugin\UiPatterns\Source\ComponentSource is still in ComponentLibraryPanel.

🇫🇷France pdureau Paris

This is a nice improvement for folks configuring UI/page builders!

♥️

Committed 9e51e1e and pushed to 11.x. Thanks!

🇫🇷France pdureau Paris

It is a surprising move, but thanks you.

However, but there are still mentions of "internal" in descriptions. "No UI" is already descriptive enough and I am afraid "internal" will add confusion.

🇫🇷France pdureau Paris

However, how do we know if the field is effectively used for override by the contributor?

I would prefer to rely on ListInterface::isEmpty() because i don't see the

When we will be decided, the scope of this issue may be only:

  • a new display_builder field widget plugin for ui_patterns_source
  • a new override optional string enum setting in ui_patterns_source instance or storage
  • a mechanism to generate automatically tabs in content view/edit/delete tabs

however, what is happening if 2 fields are override the same display?

Oops! We need to address this too :) Christian prefer to store the link between a display and a display_builder field in the entity view display config entity instead on the ui_patterns_source instance or storage

So, if we do that, we will need:

  • a new display_builder field widget plugin for ui_patterns_source
  • a new overriden_by optional string enum setting in entity view display config entity
  • a mechanism to generate automatically tabs in content view/edit/delete tabs
🇫🇷France pdureau Paris

If the need is to show what are the values used in stories, some suggestions for a flatter and smaller snippet:

  • YAML instead of JSON.
  • only the content of slots & props
🇫🇷France pdureau Paris

Finally, we have 2 components: table & cell because of this kind of markup:

fr-cell--top : Alignement vertical en haut.
fr-cell--bottom : Alignement vertical en bas.
...

And maybe other fr-cell modifiers, lets check: https://www.systeme-de-design.gouv.fr/version-courante/fr/composants/tab...

By the way, can we also add the block class (fr-cell)? Missing from examples but not BEM compliant.

There is also fr-col--xs, fr-col--sm, fr-col--md or fr-col--lg in 1.14 (not documented but found in the preview). But that doesn't necessary mean it will be an additional component, because it follows HTML logic with colgroup & col, so a mechanism inside table component.

It would be great to test those 2 components with Views, to check if UI Patterns 2 is already able to manage tables without specific row component.

🇫🇷France pdureau Paris

Add a CSS variables API Active has been updated. Discussions about design tokens are moving there.

🇫🇷France pdureau Paris

Following discussions with @d34dman and @4aficiona2 in 🌱 [Meta] Make Drupal the first "design-system native" CMS + Unify & simplify render & theme systems Active and with @e0ipso and @penyaskito on slack, I have renamed the issue and added a description of https://www.drupal.org/project/design_tokens

We now have 2 great starting points:

🇫🇷France pdureau Paris

We should centralized.

Love that. If it is centralized, we may make the transition to the Drupal Core's implementation of HTMX easier. See 📌 Follow Core's HTMX integration Active

Production build 0.71.5 2024