Having a set of example components to build against and test in various experiences will help to make sure experience builder supports as many different component building approaches as possible.

The MR adds example components that we can use to experiment/test/validate component config/data approaches while building out experience builder. See #9 for more nuance.

To start I've added a handful of components that build out the proposed supported approaches from #3446083: Document supported component modeling approaches. There's a handful of straightforward components that are used to build up the complex components. The complex components included in the MR are:

How do I test these components?

The submodule provides plugin blocks for each of the complex components above that are automatically placed on the frontpage. With default content that can be changed by updating the textarea json fields provided in the block forms.
This MR was not merged until the necessary infrastructure existed in the XB module to create and store a component tree. This issue/MR includes only a Component config entity in its default config (i.e. to make an SDC available in XB) for the components/containers/two_column SDC. #3463999: Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria will make it possible to place more of these components.

XB's default component tree for the article content type in the Standard install profile has been updated to use those 2 columns, which means that now for the very first time, XB's default layout is an actual tree, with components in slots of another component:

Before
After

⚠️ Note: not every of these components will necessarily work in XB immediately after this MR is merged! Having the components landed simplifies discoverability of unknown unknowns, and helps make abstract problems concrete 👍

Unexpected consequences

Note: The side by side appears to have already found a bug in SDC which I've opened #3446933: SDC incorrectly throws an exception about embedded slots to resolve. For now when turning on the submodule your frontpage will throw an SDC error. To get past this you can either comment out

core/lib/Drupal/Core/Template/ComponentNodeVisitor.php:165

      $error_messages[] = sprintf(
        'We found an unexpected slot that is not declared: [%s]. Declare them in "%s.component.yml".',
        implode(', ', $undocumented_ids),
        $component->machineName
      );

or disable the "SDC: Side by Side" block on the block layout page.

Why are there shoelace components?

I wanted to keep the actual html/js/css for components as simple as possible. By leveraging an already existing web component components the supporting code required for components stays small while allowing the examples to function like expected so issues can be spotted easily.

What's with all the todo's?

I added many todos to cover some of the intentions of the slots and props that are not yet supported by our APIs, like which slots should be dropzones, an example of how we could layout complex components in the SDC file, and $refs in the schema to embed definitions of other components in the complex one.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ctrlADel created an issue. See original summary.

wim leers’s picture

🤩

ctrladel’s picture

Issue summary: View changes
Related issues: +#3446933: SDC incorrectly throws an exception about embedded slots
StatusFileSize
new318.85 KB
new19.87 KB
new10.15 KB
wim leers’s picture

I tried to get this to work but ran into some challenges. I get that there are a bunch of @todos such as

#      $ref: "json-schema-definitions://experience_builder_example_components.module/two_column"
      # @todo refs don't seem to work yet

(Yep, that only works in the https://git.drupalcode.org/project/experience_builder/-/tree/research__d... branch right now!)

But I did expect that I'd be able to kinda get this branch to work, since you didn't share any caveats/instructions on how to get it to work 😅

This seems to be triggering a lot of SDC schema validation errors. I spent a good while stepping through the SDC schema validation process and fixed a few — the first one:

Twig\Error\SyntaxError: An exception has been thrown during the compilation of a template ("We found an unexpected slot that is not declared: [column_one, column_two]. Declare them in "side_by_side.component.yml"."). in Twig\Environment->compileSource() (line 2 of modules/contrib/experience_builder/modules/experience_builder_example_components/components/compound/side_by_side/side_by_side.twig).

required a work-around that I think goes against the intent of what you were trying to convey.

The second change (a required label prop that AFAICT should've been optional) seems just a minor oversight.

But then the third:

Drupal\Core\Render\Component\Exception\InvalidComponentException: [width] String value found, but an object is required/n[width] Does not have a value in the enumeration [25,33,50,66,75] in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).

had me to step through a significant part of the schema validation process to make sense of what was going on.

So … rather than me spending multiple hours trying to figure this out while being uncertain of the intended direction … could you perhaps provide some additional context, or just instructions on how to see the whole thing in action? 😇🙏

EDIT: cross-posted with #4 — yeah, I definitely don't get to see that point 😅

ctrladel’s picture

Can you give it another try? I pushed a fix for the width issue, looks like something I broke while doing clean up before opening the MR.

pdureau’s picture

Hi,

As I already did for a few themes:

Here is some feedbacks about the SDC components which may be helpful.

my-banner

3 (ctaText, ctaHref & ctaTarget) of the 5 props are not related to the banner but the CTA which is hardcoded in the template:

{% include 'experience_builder_default_design_system:my-cta' with { text: ctaText, href: ctaHref, target: ctaTarget } only %}

Also, ctaText prop definition is less strict than my-cta's href prop definition.

Injecting the CTA component through a slot would prevent this kind of issue and make the implementation cleaner.

About this:

{% if image is not empty %}

It is better to use {% if image %} instead to catch when the image is missing or resolved to false for other reasons.

image prop looks like an URL (iri-reference) instead of a plain string.

my-button

text prop may be better as a slot, to host any renderable.

my-cta

href prop : uri JSON schema format is too restrictive (relative-reference & internationalization are not allowed). It is better to use iri-reference.

{% if target is not empty %}: again, it is better to use {% if image %} instead to catch when the image is missing or resolved to false for other reasons.

<a {{ attributes }} href="{{ href }}">: it is better to do <a {{ attributes.setAttribute(href, href) }}> to avoid attribute duplication.

text prop may be better as a slot, to host any renderable.

ctrlADel changed the visibility of the branch experience_builder-3446722-2 to hidden.

ctrladel’s picture

@pdureau this branch/MR was created before there was a 0.x and there were some previous components pulled from SDC at the top level which it looks like you reviewed. I just rebased the MR onto 0.x so now the only example components are the ones in the experience_builder_example_components submodule, would be great to get your thoughts on those.

Knowing that XB will likely require some changes to SDC schemas and that there are many features that need to be built on top of the schemas the overall goal here is to provide a diverse set of example implementation approaches to ensure that XB doesn't unintentionally limit how components can be built/used. A smaller set of example components also has the benefit of being easier to update and adjust as extra settings are added/removed from the schema. The full design system implementations you mentioned will be great to use as a more thorough test once XB is a bit better defined.

Would be interested to know If there are any common component structures/approaches you use that are missing from the examples and what would be a good example we could use to add them in.

nod_’s picture

if it's for testing it should probably be in a testing module no? If it's in an example module it makes it seem like that's what we recommend doing vs. what's possible to do.

pdureau’s picture

now the only example components are the ones in the experience_builder_example_components submodule, would be great to get your thoughts on those.

Thanks. I would be pleased to have a look.

Would be interested to know If there are any common component structures/approaches you use that are missing from the examples and what would be a good example we could use to add them in.

We (UI Suite people) are working on a component template validator which will catch many of those feedbacks, but not all of them. It will be ready this summer.

wim leers’s picture

I never responded here since DrupalCon, but I never forgot about @ctrlADel's awesome work here! 😅

I've been wanting to get to this issue, and this issue's MR, which is now the oldest open MR for XB 😬

To be able to merge this MR, the field storage pieces that https://git.drupalcode.org/project/experience_builder/-/merge_requests/15 (no d.o issues yet back then) added and the subsequently added terribly hacky editing experience to demonstrate the data model ( https://git.drupalcode.org/project/experience_builder/-/merge_requests/20 aka #3452497) need to first grow to allow an actual component tree to be defined.

There's no support for nesting components currently.

I hope to continue pushing this forward next week, but chances are it'll be longer still before I get to this. Because in the very short term, even being able to edit a single component's props completely through the UI has many other moving parts that need to be addressed.

But rest assured, I will get to this 👍😊

wim leers’s picture

Status: Active » Postponed
Parent issue: » #3453690: [META] Real-time preview: supporting back-end infrastructure
wim leers’s picture

Title: Create an example set of SDC components » [PP-1] Create an example set of SDC components
Assigned: wim leers » Unassigned

Blocked on #3455728: FieldType: Support storing component *trees* instead of *lists* — otherwise the scope of this MR would be far too big.

kristen pol’s picture

@pdureau

We (UI Suite people) are working on a component template validator which will catch many of those feedbacks, but not all of them. It will be ready this summer.

Sounds great! Is this based on this work?

https://git.drupalcode.org/project/ui_patterns/-/tree/2.0.x/modules/ui_p...

pdureau’s picture

Hi Kristen,

This sub-module: ui_patterns/-/tree/2.0.x/modules/ui_patterns_devel

  • You can clone the 2.0.x branch
  • Once the module installed, you can go /admin/reports/ui-components
  • We are still tweaking the rules and messages, but the "framework" is already there and working
  • We will add a drush command later

Also, we have a nice submodule for browsing SDC components in library pages: ui_patterns/-/tree/2.0.x/modules/ui_patterns_library

michaellander’s picture

Is it worth including some examples to pressure test on the extreme end of low-code customization? For example a heading where you can adjust font-style, font-weight, heading level, margins(top, bottom, left, right), padding(top, bottom, left, right), display, position(top, bottom, left, right), text properties(color, align, decoration, etc), height, width, line-height, etc etc...

This would be both useful from performance/storage perspective, but also in some UI decisions.

kristen pol’s picture

Thanks @pdureau :)

wim leers’s picture

Title: [PP-1] Create an example set of SDC components » [PP-1] Introduce an example set of representative SDC components
Priority: Major » Critical

#3455728: FieldType: Support storing component *trees* instead of *lists* is in.

@tedbow is working on #3460856: Create validation constraint for ComponentTreeStructure. That will ensure that the tree structure for components with slots is strictly validated 👍

Once that's in, rebase Kyle’s aka @ctrlADel’s MR here, which adds components with slots. Once rebased:

  1. update the default XB field value so that an article uses Kyle's side-by-side component by default, and has two of the currently existing default components side-by-side (one in each slot)
  2. that will require updates to \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::resolveComponentProps() and \Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated
  3. … but not to \Drupal\experience_builder\Plugin\DataType\ComponentPropsValues nor \Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure

Strictly speaking, that would be out of scope, but this issue is the one with an existing MR that has slots for its SDCs, so it's the perfect place to do those last pieces of work after #3460856 is done :)

wim leers’s picture

Assigned: Unassigned » tedbow

@tedbow will be working on #20 once he finishes the issue blocking this one: #3460856: Create validation constraint for ComponentTreeStructure. I just reviewed it, and it's close 👍

wim leers’s picture

Title: [PP-1] Introduce an example set of representative SDC components » [PP-1] Introduce an example set of representative SDC components, including
Related issues: +#3463986: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components

While Ted starts working on this, #3463986: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components can happen in parallel. Then this issue updating \Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated will automatically result in SdcController::preview() supporting actual component trees too 👍

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

wim leers’s picture

Title: [PP-1] Introduce an example set of representative SDC components, including » Introduce an example set of representative SDC components, including
Status: Postponed » Needs work
wim leers’s picture

wim leers’s picture

Title: Introduce an example set of representative SDC components, including » [PP-1] Introduce an example set of representative SDC components, including

… I just realized something: #3463986-5: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components 🙈 — but hey, it'll make the remaining work here smaller 😊

tedbow’s picture

Title: [PP-1] Introduce an example set of representative SDC components, including » Introduce an example set of representative SDC components, including
kopeboy’s picture

I would add at least a default component that has a form input, like an add to cart block or a donate/deposit/withdraw one.

pdureau’s picture

@pdureau this branch/MR was created before there was a 0.x and there were some previous components pulled from SDC at the top level which it looks like you reviewed.

Thanks @ctrlADel for the information. I got a look and here are some feedbacks.

experience_builder:image

$refs resolution don't seem to work yet in Experience Builder, so the component was not fully tested, but the component template is surprising:

<img src="{{ image.src }}" alt="{{ image.alt }}" width="{{ image.width }}" height="{{ image.height }}"></img>    

What is the purpose of this component?

  • it has a single prop, also called "image", so the component looks like a meaningless wrapper around this single prop
  • it doesn't use the expected attributes object
  • no HTML classes (and the component has no dedicated CSS), so no specific UI purpose
  • it is nothing more than a rigid HTML element, like an image renderable but less powerful because with hardcoded attributes (so not compatible with lazy loading or any API working with images)

Am I missing something?

my-experience_builder:hero

If there is already a class attribute in attributes, two class attributes will be printed:

<div {{ attributes }} class="my-hero__container">

It would be better to do:

<div {{ attributes.addClass("my-hero__container") }}>

Adding attributes in the prop definition is not harmful, but not useful because always automatically added:

    attributes:
      type: Drupal\Core\Template\Attribute
      name: Attributes

No slots, is it normal? heading, subheading, cta1 & cta2 look like slots.

heading have both "required" and "minLength: 2"

experience_builder:my-section

The template doesn't use the expected attributes object.

The heading prop machine name and label doesn't match. Is it "heading" or "text"?

    text:
      type: string
      title: Heading

No slots, is it normal? text looks like a slot.

text have both "required" and "minLength: 2"

Example components

I guess the current issue is not about the previously checked components but about the ones from the experience_builder_example_components module.

But there are still 41 "todo" annotations. Is it too early to have such a look?

tedbow’s picture

Issue summary: View changes
StatusFileSize
new16.89 KB

I am trying to update `config/optional/field.field.node.article.field_xb_demo.yml` use the new Side By Side component

I am able to fill the slots correctly I think but not sure what to do with the properties, specifically what to put for Heading property
for example

modules/experience_builder_example_components/components/compound/side_by_side/side_by_side.component.yml has

    heading:
      title: Heading
#      $ref: "json-schema-definitions://experience_builder_example_components.module/heading"
      # @todo refs don't seem to work yet
      type: object

is " # @todo refs don't seem to work yet" stil right
If I uncomment the "$ref:" line I get

TypeError: Drupal\experience_builder\SdcPropJsonSchemaType::from(): Argument #1 ($value) must be of type string, null given in Drupal\experience_builder\SdcPropJsonSchemaType::from() (line 205 of modules/contrib/experience_builder/src/FieldForComponentSuggester.php).
Drupal\experience_builder\FieldForComponentSuggester->getRawMatches('experience_builder_example_components:side_by_side') (Line: 59)
Drupal\experience_builder\FieldForComponentSuggester->suggest('experience_builder_example_components:side_by_side', Object) (Line: 284)
Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem->getAvailablePropSourceChoices() (Line: 74)
Drupal\experience_builder\Plugin\Field\FieldWidget\TwoTerribleTextAreasWidget->formElement(Object, 0, Array, Array, Object) (Line: 459)
Drupal\Core\Field\WidgetBase->formSingleElement(Object, 0, Array, Array, Object) (Line: 219)
Drupal\Core\Field\WidgetBase->formMultipleElements(Object, Array, Object) (Line: 120)
Drupal\Core\Field\WidgetBase->form(Object, Array, Object) (Line: 190)
Drupal\Core\Entity\Entity\EntityFormDisplay->buildForm(Object, Array, Object) (Line: 121)
Drupal\Core\Entity\ContentEntityForm->form(Array, Object) (Line: 134)
Drupal\node\NodeForm->form(Array, Object) (Line: 107)
Drupal\Core\Entity\EntityForm->buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 536)
Drupal\Core\Form\FormBuilder->retrieveForm('node_article_form', Object) (Line: 284)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
....

Leaving it commented out for now

Not sure how to fill that out in the props array. In the editor when I adding an article I get

⚠️ This component prop has a shape that has no equivalent in Drupal fields — yet.

if I try to added an article with default values, plus uploading 2 images, for the fields I get

TypeError: Twig\TemplateWrapper::render(): Argument #1 ($context) must be of type array, null given, called in /Users/ted.bowman/sites/exp-d-core/vendor/twig/twig/src/Extension/CoreExtension.php on line 1332 in Twig\TemplateWrapper->render() (line 36 of vendor/twig/twig/src/TemplateWrapper.php).
Twig\Extension\CoreExtension::include(Object, Array, 'experience_builder_example_components:heading', NULL, ) (Line: 56)
__TwigTemplate_b267b61305cf51ea791258bf2a6a18e5->{closure}() (Line: 1775)
Twig\Extension\CoreExtension::captureOutput(Object) (Line: 53)
__TwigTemplate_b267b61305cf51ea791258bf2a6a18e5->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array, Array) (Line: 125)
__TwigTemplate_a43b7fe6eafe0515efeef249f410eb00___1303851157->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array) (Line: 40)
__TwigTemplate_a43b7fe6eafe0515efeef249f410eb00->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array) (Line: 335)
Twig\Template->render(Array) (Line: 38)
Twig\TemplateWrapper->render(Array) (Line: 234)
Drupal\Core\Template\TwigEnvironment->renderInline('{# inline_template_start #}{# This template was dynamically generated by single-directory components #}
{% embed 'experience_builder_example_components:side_by_side' %}
{% block column_one %}
{{ column_one }}
{% endblock %}
{% block column_two %}
{{ column_two }}
{% endblock %}
{% endembed %}
', Array) (Line: 54)
Drupal\Core\Render\Element\InlineTemplate::preRenderInlineTemplate(Array)

So seems like the expected problem of modules/experience_builder_example_components/components/compound/side_by_side/side_by_side.component.yml not sending any value on to modules/experience_builder_example_components/components/simple/heading/heading.component.yml

When I look at the page output of EndToEndDemoIntegrationTest when fails this is the same.

Basicalyy

tedbow’s picture

Talked to @Wim Leers briefly about this and we agreed it will better to change the Side By Side to simplify or remove the props and focus on getting the slots working.

will work on that

kristen pol’s picture

I'm unfamilar with "shoe" design things so I asked chatgpt about it... would love more explanation :)

In a design system, the term "shoe button" isn't a widely recognized standard term. It could be a specific term used within a particular organization or design system to refer to a unique button style or function. In the context of user interface design, buttons can have various styles and functionalities, often named descriptively based on their appearance or use case.

This is in the summary but I don't know what that means... where are these from? Sorry if this is obvious :)

Why are there shoelace components?
I wanted to keep the actual html/js/css for components as simple as possible. By leveraging an already existing web component components the supporting code required for components stays small while allowing the examples to function like expected so issues can be spotted easily.

pdureau’s picture

Hi Kristen,

I understood "shoe" is about https://shoelace.style/ here.

tedbow’s picture

@Wim Leers re #31 and #20 I think I am going to switch from using modules/experience_builder_example_components/components/compound/side_by_side to using
modules/experience_builder_example_components/components/containers/two_column this is simpler example. Side by Side passes object to sub components and two_column simply gives uses 2 slots to put components in .

wim leers’s picture

#34: WFM!

tedbow’s picture

  1. re #20 and work @Wim Leers laid out

    update the default XB field value so that an article uses Kyle's side-by-side component by default, and has two of the currently existing default components side-by-side (one in each slot)

    I changed this to use the Two Column see #34

    that will require updates to \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::resolveComponentProps() and \Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated

    No changes were needed. I think because the changes needed were done in #3463986: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components

  2. \Drupal\Tests\experience_builder\Functional\EndToEndDemoIntegrationTest::test passes for me locally but fails on gitlab because $tree->getComponentInstanceUuids() seems return the UUIDs in different order. I am giong to add a sort so this is predictable.
  3. I can't get \DefaultConfigTest::testConfig to pass. I tried to debug this but can't see a difference in the values
wim leers’s picture

#36.2: 🤔 How is that possible? \Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure::getComponentInstanceUuids() does this:

return array_column($this->getComponents(), 'uuid');

… and it's coming from a JSON blob that was deserialized. How can the order possibly change?! If the order can change, that would also mean the component tree could randomly shuffle around?! 😱

What am I missing?

wim leers’s picture

Also, for some reason these very simple steps that manually reproduce EndToEndDemoIntegrationTest no longer work:

vendor/bin/drush si standard --account-name=root --account-pass=root --yes
vendor/bin/drush pm:install experience_builder --yes

(They're documented at the top of /CONTRIBUTING.md.)

The node.article.field_xb_demo field instance is simply not installed. I've tried it twice now, and I'm on the latest commit: 5b192757b1672a16a3fa227938cfe0e8549a3915. No relevant log messages after those two steps.

Any idea what's going on?

wim leers’s picture

#36.3: I’ve had to debug that DefaultConfigTest before too — with the right conditional breakpoint it’s totally doable. So far it’s never lied to me. But the failing test output can sure be confusing.

tedbow’s picture

This was because I needed add experience_builder_example_components as a dependency of the main module since it is needed for the default value of the field_xb_demo now. See my last commit

wim leers’s picture

Title: Introduce an example set of representative SDC components, including » Introduce an example set of representative SDC components; transition from "component list" to "component tree"
wim leers’s picture

I did a high-level review of @tedbow's changes. I didn't like how the SDCs that @ctrlADel built had started deviating significantly.

So:

  1. I merged in origin/0.x to get #3462709: Introduce `hook_storable_prop_shape_alter()`, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed, which introduced some crucial bugfixes to massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation().
  2. That in turn allows reverting the change @tedbow made to the side-by-side component to not use a integer + enum.
  3. … but that revealed a limitation of the JSON encoding (which is also true of the YAML encoding): all keys are strings. But the keys were meant to be the values. Result: 50 as a choice would always be represented by '50': a string, not an integer!
  4. In debugging that, I discovered ::storageSettingsFromConfigData(), which was added ~10 years ago for addressing precisely this problem. Whew! 😅 Fix: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
  5. In #3461499, I added fallback logic to ComponentPropsForm to pick the appropriate widget while we await follow-up issues to land (there's explicit @todos in place for that). But I did not update TwoTerribleTextAreasWidget accordingly. And that's what @tedbow was running into here. Fixed that: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
  6. But then I found another bug, this time one deep in Drupal core: ListIntegerItem and its default widget actually store an string value … 🙃 It's missing the explicit cast it needs: calling getValue() on a ListIntegerItem actually returns '50', not the 50 its allowed values specified! 🤯 Fixed that in https://git.drupalcode.org/project/experience_builder/-/merge_requests/2....

PHPUnit tests are passing again after I first broke them 😊

So, now @tedbow can start his week (morning, Ted! ☕️) with something closer to what @ctrlADel envisioned 👍

I think we should split off https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... + https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... into its own issue — I'll ask Utkarsh to do that 😊

P.S.: another reason for me to contribute some commits and nudge this closer to the original intent, is that this might otherwise have identified another upstream issue for #3462705: [META] Missing features in SDC needed for XB.

kristen pol’s picture

🎉 Y’all going to find and fix all the core bugs 😂

tedbow’s picture

Putting notes here for myself tomorrow or @Wim Leers if you wanted to take a look. But I will look tomorrow if no one else does. Below is as far as I was able to debug before my end of day

Just the cypress e2e test failing now. I think this because \Drupal\experience_builder\Controller\SdcController::preview is not sending back results with the slots.

If I make a node and visit node/1 I get the components rendered in the 2 column layout. But if I visit xb/node/1 I see the default values for the slots of modules/experience_builder_example_components/components/containers/two_column/two_column.twig "The contents of the one column." and "The contents of the two column."

Putting debug points in \Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated::toRenderable
when I go to node/1 in here

$renderable_component_tree = $this->getValue();

results in the slots being filled and $renderable_component_tree->getContent() is

{"a548b48d-58a8-4077-aa04-da9405a6f418":{"two-column-uuid":{"component":"experience_builder_example_components:two_column","props":{"width":50},"slots":{"column_one":{"dynamic-image-udf7d":{"component":"experience_builder:image","props":{"image":{"src":"public:\/\/2024-08\/heading-no-field-for-shape_6.png","alt":"asdf","width":614,"height":206}}},"static-static-card1ab":{"component":"experience_builder:my-hero","props":{"heading":"hello, world!","cta1href":"https:\/\/drupal.org"}}},"column_two":{"dynamic-static-card2df":{"component":"experience_builder:my-hero","props":{"heading":"asdfasdf","cta1href":"https:\/\/drupal.org"}},"dynamic-dynamic-card3rr":{"component":"experience_builder:my-hero","props":{"heading":"asdfasdf","cta1href":"public:\/\/2024-08\/heading-no-field-for-shape_6.png"}},"dynamic-image-static-imageStyle-something7d":{"component":"experience_builder:image","props":{"image":{"src":"http:\/\/exp-d-core.test\/sites\/default\/files\/styles\/thumbnail\/public\/2024-08\/heading-no-field-for-shape_6.png.webp?itok=FuRwXRQg","alt":"asdf","width":100,"height":34}}}}}}}}

when I go to xb/node/1

Putting debug points in \Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated::toRenderable
when I go to xb/node/1 in here

$renderable_component_tree = $this->getValue();

results in the NOT slots being filled and $renderable_component_tree->getContent() is just
{"a548b48d-58a8-4077-aa04-da9405a6f418":{"two-column-uuid":{"component":"experience_builder_example_components:two_column","props":{"width":50}}}}
So \Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated::renderify does not call itself recursively to fill the slots as "slots" is not present.

wim leers’s picture

If I make a node and visit node/1 I get the components rendered in the 2 column layout.

I do too, but it looks awfully off, because the first column is 3456 pixels wide, whereas the second is only 140 pixels wide? 😅 (Fresh install, browser caches wiped.) → 🐛

But if I visit xb/node/1 I see the default values for the slots of modules/experience_builder_example_components/components/containers/two_column/two_column.twig "The contents of the one column." and "The contents of the two column."

Reproduced.

That's because the UI/client provides this request body:

{
  "layout": {
    "uuid": "root",
    "type": "root",
    "name": "root",
    "children": [
      {
        "uuid": "two-column-uuid",
        "nodeType": "component",
        "type": "experience_builder_example_components:two_column"
      }
    ]
  },
  "model": {
    "two-column-uuid": {
      "width": 50,
      "name": "Two column"
    }
  }
}

… because that is what the client received as a response from /api/layout/node/1.

That's why you observe different behavior:

  • visiting /node/1 results in the stored component tree being loaded and hydrated and then rendering correctly
  • using the XB UI at /xb/node/1 results in the incorrect layout (top level only: a list, not a tree) being passed from server to client (you need to update SdcController::layout() to transform the server-side data structure to the client-side equivalent), and that same incorrect layout is then sent to the server for previewing, which is transformed back to the server-side data structure, but since it is incomplete, you get a weird hydrated tree: the tree the client sent because the server provided it

IOW: this MR still needs to update \Drupal\experience_builder\Controller\SdcController::layout()#3463986: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components only updated SdcController::preview(). You essentially now need to write the inverse of SdcController::clientLayoutToServerTree(), which is a helper I introduced in that issue.

(Eventually the data models might become closer to each other or even the same, but for now, that's not the case. I started pushing #3450308: Create an Open API spec for the current mock HTTP responses forward because it has not been finished yet, and will help clarify this.)

The fact that you're at this point now means you're probably in the final stretch! 😄🥳

P.S.: where you wrote when I go to node/1 in here, that should've been /xb/node/1. Fortunately it was clear from context 👍

tedbow’s picture

@Wim Leers thanks for the context

P.S.: where you wrote when I go to node/1 in here, that should've been /xb/node/1. Fortunately it was clear from context 👍

fixed

tedbow’s picture

  1. Ok. now I think I have handled the problems I mentioned in #45 and @Wim Leers response in #46
  2. Currently I can now go to xb/node/1 and see the components being rendered. Also SdcController::wrapComponentsForPreview is now working on the nested components in the slots. When I hover over the components I can see the highlight outline which I think depends on the div added by wrapComponentsForPreview
  3. This does get more of the Cypress tests passing. You can see before my latest push of commits there were 4 screenshots for failed tests in a11y.cy.js and the now there is just 1 🎉
  4. The last failed test for a11y.cy.js is for the form loading. I have confirmed locally the forms do not load when you click a component. Debugging at \Drupal\experience_builder\Form\ComponentPropsForm::buildForm and trying this on 0.x and the MR I think this because on the MR for some reason tree is being sent back to the server as an empty array when trying create the form.

    Maybe something needs to change on the client, I am not sure

  5. There are a few more Cypress test failing but I have not had a chance to investigate them
tedbow’s picture

With some help from @hooroomoo I was able to figure where the JS that caused #48.3

I pushed up some hacky JS to get the forms to work. I don't have time to see if this causes eslint problems, but probably. Can fix next. Will see if it passes the Cypress test. Locally the forms open now🎉

tedbow’s picture

  1. So my previous JS commits broke UI build gitlab job, so the Cypress tests didn't run
  2. Fixed that got the UI build gitlab job passing
  3. The Cypress tests ran again.
  4. All the cypress tests in a11y.cy.js are now passing 🎉
  5. The fix for a11y.cy.js also fixed a couple other tests. Went from 7 passing, 4 failing to 9 passing, 2 failing 👍
  6. I haven't had a chance to look at the remaining 2 failing Cypress tests. I am up for trying to fix these so will leave the issue assigned to me but if someone else with more express with Cypress want to get these passing that is fine too. but it might need some server side changes also
tedbow’s picture

Assigned: tedbow » wim leers
Status: Needs work » Needs review
  1. Fixed add-section.cy.js

    Just add to change to clicking the containing Two Column component.

    Someone should probably check the functionality manually to see if still works as intended.

  2. 1 more Cypress test failing, xb-general.cy.js. It is currently failing because it didn't expect the new "Two Column" component in the menu. I think I fixed that but there are a lot of other assertions below that. So it will probably fail somewhere else.
  3. Fixing the test would go much faster if I could run Cypress tests locally so I will look into that tomorrow.
  4. UPDATE: xb-general.cy.js just passed. Gitlab Pipeline is ✅. 🎉

@Wim Leers I don't have time to self review of this now, so I am sure there is stuff I need to clean up/fix. Assigning to you if you want to take a look.
but if you would rather look after I do a self review feel free to assign it back to me.

wim leers’s picture

Assigned: wim leers » tedbow
Status: Needs review » Needs work
StatusFileSize
new528.34 KB

Thanks SO MUCH for #48 + #49 + #50 + #51, that made this much easier to follow 😊🙏

First of all: just testing this branch gets me this, which is AWESOME 🤩:

Review

  1. 🤔 The changes made by you after @ctrlADel's last commit (de8cbb170e95ac6edf0c67dc1c92d008041bba21) should not remove anything from the original intent, and ideally resolve all @todos he added. Looking at git d de8cbb170e95ac6edf0c67dc1c92d008041bba21 -- modules/experience_builder_example_components. You did not deviate from the intent, but there's one thing left to be done, see next point.
  2. 🙏 👆 reveals $refs stuff is not yet updated. But we're lacking specifics for Kyle's intent there: he is using $refs but did not specify the corresponding schema 😅. He did not write the corresponding schema.json file, but he did write the corresponding Twig. Which means you can write the necessary schema, Ted. See modules/experience_builder_example_components/components/simple/heading/heading.twig for example: that reveals there's two inputs Kyle was imagining for heading: element and text. I assume text would be type: string and element would be type: string, enum: [h2, h3, h4, h5, h6]. You can derive his intent for the other $refs in a similar way.
  3. 🙏 Once that's done, I think it's time to remove the blocks he added — which I said in my previous review too: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
  4. ✅ The changes to the Cypress E2E tests look 👍
  5. ✅ The changes to EndToEndDemoIntegrationTest look 👍

That second bullet is a fair bit of work, but this is getting close! 😊🥳

wim leers’s picture

wim leers’s picture

The Cypress E2E test failures are legitimate — did you know you can see the screenshots it made of failures on GitLab CI? 😊

tedbow’s picture

Not sure why EndToEndDemoIntegrationTest is failing. Seems that can't a widget "width" property of the two_column component

Will debug more tomorrow

wim leers’s picture

Not sure why EndToEndDemoIntegrationTest is failing. Seems that can't a widget "width" property of the two_column component

The reason for that: the logic that #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings introduced does not resolve $ref.

Fixed that for you: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

wim leers’s picture

In https://git.drupalcode.org/project/experience_builder/-/merge_requests/2..., you deleted config/optional/field.field.node.article.field_xb_demo.yml, which also results in all Cypress E2E tests failing … because the necessary defaults are no longer set up, at least not without also installing the new "example components" module.

Why not go with what I suggested yesterday, which would fix that entire problem? I wrote:

Also, if it makes things simpler, then I say: move all these schema additions to experience_builder's own schema.json for now. I know that @lauriii has talked previously about having a standard set of object shapes/schema definitions that contrib SDCs should be able to adopt, because that'll allow more cooperation/mixing-and-matching.

I think that's the more pragmatic choice? 😅 And it will ensure that @lauriii and the rest of us must have that crucial conversation. 👍

P.S.: also asked @Utkarsh_33 to help make this MR much smaller by landing 49 of the 132 file changes in #3467046: Move /submodules to /modules 👍 You'll need to merge that in.

tedbow’s picture

@Wim Leers re

Also, if it makes things simpler, then I say: move all these schema additions to experience_builder's own schema.json for now. I know that @lauriii has talked previously about having a standard set of object shapes/schema definitions that contrib SDCs should be able to adopt, because that'll allow more cooperation/mixing-and-matching.

Now the Cypress tests are passing.

standard set of object shapes/schema definitions

I think it is probably a good idea. But now that we have schema.json working in the sub-module it is seems like a more realistic test case to prove that another module can provide its own schema.json and have a prop exposed in the form.

It seems like even if we have a standard set of object shapes/schema definitions modules will still need to have their own for unique cases.

wim leers’s picture

  1. +1 to your last question: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
  2. #58:

    It seems like even if we have a standard set of object shapes/schema definitions modules will still need to have their own for unique cases.

    Yes, but the difference will be that those modules' schema.json files are not being depended upon by the XB module itself. That's what makes this one special.

  3. https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... is still not yet addressed.
  4. #57 is not addressed, and hence the steps to a working environment documented in CONTRIBUTING.md no longer work.
tedbow’s picture

re #59

Yes, but the difference will be that those modules' schema.json files are not being depended upon by the XB module itself. That's what makes this one special.

This is not actually the case anymore. You can install experience_builder without experience_builder_example_components since the example field is now in experience_builder_example_components

tedbow’s picture

re #57

In https://git.drupalcode.org/project/experience_builder/-/merge_requests/2..., you deleted config/optional/field.field.node.article.field_xb_demo.yml, which also results in all Cypress E2E tests failing

Actually that moved the file to modules/experience_builder_example_components/config/install/field.field.node.article.field_xb_demo.yml

I think it makes more sense to the demo field there since it relies on the demo component. I just moved the field storage in another commit to make it cleaner.

This also means that we can easily put all new props in modules/experience_builder_example_components/schema.json

#57 is not addressed, and hence the steps to a working environment documented in CONTRIBUTING.md no longer work.

So the update docs in CONTRIBUTING.md work now with 4. `drush pm:install experience_builder experience_builder_example_components`

wim leers’s picture

Issue tags: +Needs followup

Discussed in detail with @tedbow — he'll extract the schema-related challenges in the side-by-side component out of this issue into a follow-up. Because that is not relevant for the transition from "component list" to "component tree" part of the issue title — and it's important that that lands very soon. It unblocks a lot of work for #3454094: Milestone 0.1.0: Experience Builder Demo.

tedbow’s picture

So per #62 another thing @Wim Leers I discussed was getting rid of the new sub-module and put all the components in the main module. Because we can't make experience_builder dependent on the submodule because the submodule can't be installed without experience_builder

The following problems happened when I got rid of the sub-modulea

  1. This seemed like an easy task but for some reason EndToEndDemoIntegrationTest is failing when it tries to install the module with the error below.

    The problem seems to be because the module config is being validated on install and JsonSchemaDefinitionsStreamwrapper is not being used because the module is not fully installed yet. I have set debugged and confirm \JsonSchema\Uri\UriRetriever is being used instead of JsonSchemaDefinitionsStreamwrapper. field.field.node.article.field_xb_demo.yml is being saved which depends on experience_builder.component.experience_builder+two_column which uses components/containers/two_column/two_column.component.yml. I am not clear why components/image/image.component.yml does not have the same problem in 0.x. maybe because width is integer and image is object property?

  2. Some kernel tests are also failing because they are dependent on the specific components we currently have in the main module and there not being different ones

Unless there is obvious reason for 1) I think we should be practical and commit this issue with the new sub-module and follow-up to move all the components into the main module. I know this is not ideal for people trying out the sub-module but we even do a messages on install that says "Please install the sub-module to see an example."

I know this issue is blocking front-end work and I don't think it will matter on the front-end if 1 or 2 modules need to installed. It just seems like every change uncovers a new problem and meanwhile I think solving these problem is really helping the front-end work.

JsonSchema\Exception\ResourceNotFoundException : file_get_contents(json-schema-definitions://experience_builder.module/column-width): Failed to open stream: No such file or directory /Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Uri/Retrievers/FileGetContents.php:38
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Uri/UriRetriever.php:208
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Uri/UriRetriever.php:181
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/SchemaStorage.php:52
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/SchemaStorage.php:115
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/SchemaStorage.php:138
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/SchemaStorage.php:162
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/Constraint.php:123
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/ObjectConstraint.php:145
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/ObjectConstraint.php:47
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/Constraint.php:90
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/UndefinedConstraint.php:74
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/UndefinedConstraint.php:52
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/Constraint.php:123
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/SchemaConstraint.php:92
/Users/ted.bowman/sites/exp-d-core/vendor/justinrainbow/json-schema/src/JsonSchema/Validator.php:61
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php:170
/Users/ted.bowman/sites/exp-d-core/modules/contrib/experience_builder/src/Plugin/Validation/Constraint/ValidComponentTreeConstraintValidator.php:81
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:202
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:154
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:164
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:164
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:106
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php:93
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/TypedData/TypedData.php:132
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php:109
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:89
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/Config.php:230
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:278
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Entity/EntityStorageBase.php:486
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:257
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Entity/EntityBase.php:354
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:614
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/ConfigInstaller.php:389
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/ConfigInstaller.php:260
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Config/ConfigInstaller.php:164
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/Extension/ModuleInstaller.php:326
/Users/ted.bowman/sites/exp-d-core/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/Users/ted.bowman/sites/exp-d-core/modules/contrib/experience_builder/tests/src/Functional/EndToEndDemoIntegrationTest.php:51
/Users/ted.bowman/sites/exp-d-core/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

tedbow’s picture

ok I figured out why 0.x does not have the problem that this MR has with EndToEndDemoIntegrationTest that I describe in #63.2. This is because in 0.x

  1. field.field.node.article.field_xb_demo.yml depends on 2 components but only components/image/image.component.yml uses a $ref.
  2. The components/image/image.component.yml does not throw an exception because it is being validated on install before JsonSchemaDefinitionsStreamwrapper being used but because the it uses a dynamic source type it will not throw an exception that we ignore because of the next point
  3. The exception in this MR is trigger at

    ValidComponentTreeConstraintValidator.php:81

    $props_values = $value->resolveComponentProps($component_instance_uuid);
    $component = $this->componentPluginManager->find($component_id);
    $this->componentValidator->validateProps($props_values, $component);
    

    so the last line will lead to the validate trying to use the $ref

    But for image with since it is using a dynamic prop source we can't evaluate the entity because it doesn't exist until we have a real node. So this gets ignored for default field config

    catch (\OutOfRangeException $e) {
            // DynamicPropSources cannot be validated in isolation, only in the
            // context of a host content entity.
            // @todo Create specific exceptions for this in
            //   https://drupal.org/i/3462160.
            if ($component_tree_type === 'config') {
              // Silence this exception until this config is used in a content
              // entity.
            }
    

So basically I think we found a new problem: Default field Config that uses a static prop source can't use a component that uses $ref: json-schema-definitions://experience_builder.module/.... unless the XB module is already enabled.

So basically if you had module the depends on XB then this is not a problem. Just for the XB module itself

So basically I think we should have to sub-module to sidestep this for now unless someone knows a quick fix.

The other option would be to set protected $strictConfigSchema = FALSE; for now but this might hide other problems

FWIW this problem does not happen if I install the module manually

tedbow’s picture

#64

The other option would be to set protected $strictConfigSchema = FALSE; for now but this might hide other problems

I didn't know that \Drupal\Core\Config\Development\ConfigSchemaChecker took a list of config to ignore. So maybe ignoring 'field.field.node.article.field_xb_demo' until a follow-up is ok so that we don't have to have the sub-module?

wim leers’s picture

Assigned: tedbow » wim leers

@tedbow walked me through #64 earlier today. That was really helpful!

The root of the problem is that config validation runs before stream wrappers are registered, resulting in this mind-boggling stack trace that really is just triggered by installing default config 😅


1) Drupal\Tests\experience_builder\Functional\EndToEndDemoIntegrationTest::test
JsonSchema\Exception\ResourceNotFoundException: file_get_contents(json-schema-definitions://experience_builder.module/column-width): Failed to open stream: No such file or directory

/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Uri/Retrievers/FileGetContents.php:38
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Uri/UriRetriever.php:208
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Uri/UriRetriever.php:181
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/SchemaStorage.php:52
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/SchemaStorage.php:115
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/SchemaStorage.php:138
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/SchemaStorage.php:162
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/Constraint.php:123
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/ObjectConstraint.php:145
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/ObjectConstraint.php:47
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/Constraint.php:90
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/UndefinedConstraint.php:74
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/UndefinedConstraint.php:52
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/Constraint.php:123
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/SchemaConstraint.php:92
/Users/wim.leers/core/vendor/justinrainbow/json-schema/src/JsonSchema/Validator.php:61
/Users/wim.leers/core/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php:170
/Users/wim.leers/core/modules/contrib/experience_builder/src/Plugin/Validation/Constraint/ValidComponentTreeConstraintValidator.php:81
/Users/wim.leers/core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:202
/Users/wim.leers/core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:154
/Users/wim.leers/core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:164
/Users/wim.leers/core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:164
/Users/wim.leers/core/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:106
/Users/wim.leers/core/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php:93
/Users/wim.leers/core/core/lib/Drupal/Core/TypedData/TypedData.php:132
/Users/wim.leers/core/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php:109
/Users/wim.leers/core/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:89
/Users/wim.leers/core/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/Users/wim.leers/core/core/lib/Drupal/Core/Config/Config.php:230
/Users/wim.leers/core/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:278
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/EntityStorageBase.php:486
/Users/wim.leers/core/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:257
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/EntityBase.php:354
/Users/wim.leers/core/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:613
/Users/wim.leers/core/core/lib/Drupal/Core/Config/ConfigInstaller.php:389
/Users/wim.leers/core/core/lib/Drupal/Core/Config/ConfigInstaller.php:260
/Users/wim.leers/core/core/lib/Drupal/Core/Config/ConfigInstaller.php:164
/Users/wim.leers/core/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
/Users/wim.leers/core/core/lib/Drupal/Core/Extension/ModuleInstaller.php:326
/Users/wim.leers/core/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/Users/wim.leers/core/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:500
/Users/wim.leers/core/core/tests/Drupal/Tests/BrowserTestBase.php:575
/Users/wim.leers/core/core/tests/Drupal/Tests/BrowserTestBase.php:370
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

Ironically, \Drupal\Core\Extension\ModuleInstaller::install() does register the stream wrappers immediately after default config is installed! 😬

Fortunately, we can fix this by doing that manually in hook_module_preinstall()! 🥳 Plus, we'll be able to delete that hook implementation once #3352063: Allow schema references in Single Directory Component prop schemas is in.

So, a solution to all of #64 + #65 with a trivial hook implementation: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

Wim Leers credited lauriii.

wim leers’s picture

Issue tags: -Needs followup

First: sorry, @tedbow, I didn't quite realize just how many SDCs were in this MR. In my recollection, it was far less. Given that we have various tests that verify that XB can provide a meaningful experience, getting this across the finish line was more laborious and more broadly scoped than I anticipated. 🙈😬

Which brings me to…

@tedbow removed the side_by_side component in https://git.drupalcode.org/project/experience_builder/-/merge_requests/2..., to allow the vast majority of this MR to land without being blocked on additional details.

I have to agree with @finnsky: the purpose of the text component is very unclear to me. It's just wrapping an arbitrary string in a <div>. Perhaps it was intended to contain rich text/markup, and use CKEditor 5 as the input UX? I searched, and could not find an answer to that in neither the commits, nor the comments on this issue. So rather than bringing in the Accordion component, I omitted it: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2..., as well as the Text component: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

Created the follow-up to ensure we do not forget about them: #3467950: Follow-up for #3446722: `side_by_side` + `accordion` (+ maybe `text`) components.

Wim Leers credited finnsky.

wim leers’s picture

Issue summary updated to reflect the reality of what is in the MR that is about to be merged.

@everyone, I trust that we all prefer to see progress instead of stagnation. The spirit of this issue is the one @ctrlADel outlined in #9.

@pdureau in #11: We (UI Suite people) are working on a component template validator which will catch many of those feedbacks, but not all of them. It will be ready this summer. — looking forward to that — that'll be critical for the SDC ecosystem! 👏

Follow-up: components left out of this MR

See #68, issue: #3467950: Follow-up for #3446722: `side_by_side` + `accordion` (+ maybe `text`) components.

Follow-up: name as the name for an SDC prop

This issue/MR includes only a Component config entity in its default config (i.e. to make an SDC available in XB) for the components/containers/two_column SDC. #3463999: Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria will make it possible to place more of these components.

I tried using /admin/structure/component/add (which will soon be obsolete, see #3463999), and had to harden its logic a bit because we're exercising many more SDC prop shapes with these components added — great! 👍

In doing so, I re-discovered a bug that we only created @todos for, because it was a theoretical future, but now it is a reality: #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc..

Follow-up: SDC props of type: object with no direct match

Obvious and common use case, but we didn't have an SDC for this yet → #3467890: [later phase] Support `{type: object, …}` prop shapes with single level that require *multiple* field types: use `field_union`? — OUT OF SCOPE: nested components/component reuse

Follow-up: page hierarchy display

This was built very early on, and now that this is in place, that important UI piece can move forward: #3458503: Improve the page hierarchy display.

Far future follow-up: default design system

Eventually, all of these components will likely disappear, or perhaps evolve into the default design system. As Kyle described in #9, they fulfill a purpose of poking at the edges, and going beyond what many of the (rather simple) SDCs in Drupal core do. That eventual issue already exists: #3446052: [later phase] Create components for a default design system.

… and I'm sure there's many more that will eventually be spawned because of this. But the key results are:

  • Wim Leers committed a8ead8ae on 0.x authored by ctrlADel
    Issue #3446722 by tedbow, Wim Leers, ctrlADel, finnsky, lauriii:...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.