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?
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.
Time flies. I move to alpha4. We need 📌 Implement WithDisplayBuilderInterface in Entity View Active to be merged first anyway.
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
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 implementingDraggableListBuilder
. Add a little explanation, like Text Format. (and updateConfigFormBuilder::buildDisplayBuilder()
)
Page Manager was removed
Everything done.
Follow-up: 📌 WithDisplayBuilderInterface follow-ups Active
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
Everything done.
Follow-up: 📌 WithDisplayBuilderInterface follow-ups Active
FYI, in
📌
Align config storage properties & mechanisms
Active
is_display_builder_views
will become views:style
I take it back, i need to add little rule in ::initInstanceIfMissing()
It looks good! Thanks a lot.
Some little proposals.
Definitions
Props:
type
title is missingcontent
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 %}
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
Remain work:
- Regression, display not rendered. Looks like a context issue.
- Remove
DisplayBuilder::DISPLAY_BUILDER_CONFIG
again - Update devel controller again
Let's keep this issue and MR only for Views integration and the rest move there: 📌 Implement WithDisplayBuilderInterface in Entity View Active
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 acheckbox
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?
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
toWithDisplayBuilderInterface
- Merge
StorageProperties
enum to static constant inConfigFormBuilder
?
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()
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:
- 📌 Define form elements from SDC Active
- ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed
- ✨ Add a style utility API Active
- ✨ Add a CSS variables API Active
It is ambitious, but it is doable. Let's make Drupal the "first deisgn-system native CMS".
Restore https://git.drupalcode.org/project/ui_patterns/-/commit/1475475e8e7ec4c3...
And you can merge by yourself :)
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.
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
Waiting pipeline. Automerge will not be triggered. You can merge, it is OK
Now we have a pretty OK compoennt structure, let's check if it is usable by UI Patterns before merging.
Proposals:
- At the ViewRow level, add a new ViewRow plugin which is removing the
view_fields
theme wrapper - At the ViewRow level, alter
ViewRow::render()
outputs to removeview_fields
theme wrapper - At the ViewStyle level, remove the
view_fields
theme wrapper fromViewRowsSource::getPropValue()
@d34dman A lot of exciting work here: 🌱 Gradually replace Drupal's AJAX system with HTMX Active
No update here but this issue is still working on 👍 We are still excited and we are still targeting Drupal 11.3
thanks Wim :)
Anybody can take this ticket :)
Waiting for answer. Moved to alpha4.
Moved to alpha4 because we need 📌 Align source contexts Active before
As far as I know, Json schema allows only one value in $ref
https://json-schema.org/understanding-json-schema/structuring
CI pipeline running. Must be OK
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.
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)
It seems DisplayBuilderEntityViewDisplay
can also implements the upcoming EntityWithDisplayBuilderInterface
:)
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
Last task remaining: better switch in ThemeRegistryAlter
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}
Also, this is the opportunity of adding config dependency management
- In Entity Display: add display builder config entity
- In Views: add display builder config entity
- In Page Layout: already done in 📌 [1.0.0-beta1] Add proper page management Active
Ok, let's wait ✨ [1.0.0-alpha1] Make Island plugins configurable Active
I take it because I work on similar topics in 📌 [1.0.0-beta1] Add proper page management Active
By the way, some of the logic expected in Drupal\ui_patterns_overrides\Plugin\UiPatterns\Source\ComponentSource
is still in ComponentLibraryPanel
.
This is a nice improvement for folks configuring UI/page builders!
♥️
Committed 9e51e1e and pushed to 11.x. Thanks!
OK great, I will commit it.
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.
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 forui_patterns_source
- a new
override
optional string enum setting inui_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 forui_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
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
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.
✨ Add a CSS variables API Active has been updated. Discussions about design tokens are moving there.
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:
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