- Issue created by @ctrladel
- Merge request !2#3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" → (Merged) created by ctrladel
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
I tried to get this to work but ran into some challenges. I get that there are a bunch of
@todo
s 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 😅
- 🇺🇸United States ctrladel North Carolina, USA
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.
- 🇫🇷France pdureau Paris
Hi,
As I already did for a few themes:
- Kinetic: 🐛 SDC components: Fix JSON schema usage Needs work
- Radix: 🐛 SDC components: Fix JSON schema usage Fixed
- Prototype: 🐛 SDC components: Fix JSON schema usage Fixed
- Umami: 📌 Umami Demo: review of the already merged SDC components Fixed
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'shref
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 a slot insteadmy-cta
href
prop :uri
JSON schema format is too restrictive (relative-reference & internationalization are not allowed). It is better to useiri-reference
. - 🇺🇸United States ctrladel North Carolina, USA
ctrlADel → changed the visibility of the branch experience_builder-3446722-2 to hidden.
- 🇺🇸United States ctrladel North Carolina, USA
@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.
- 🇫🇷France nod_ Lille
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.
- 🇫🇷France pdureau Paris
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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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 👍😊
- Assigned to wim leers
- Status changed to Postponed
7 months ago 2:39pm 11 June 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Note: when working on this, I'll also port relevant pieces from 🌱 [PP-1] Create components for a default design system Postponed , if any.
- Issue was unassigned.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Blocked on 📌 FieldType: Support storing component *trees* instead of *lists* Fixed — otherwise the scope of this MR would be far too big.
- 🇺🇸United States Kristen Pol Santa Cruz, CA, USA
@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...
- 🇫🇷France pdureau Paris
Hi Kristen,
This sub-module: https://git.drupalcode.org/project/ui_patterns/-/tree/2.0.x/modules/ui_p...
- 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
- 🇺🇸United States michaellander
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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 FieldType: Support storing component *trees* instead of *lists* Fixed is in.
@tedbow is working on 📌 Create validation constraint for ComponentTreeStructure Needs work . 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:
- 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) - that will require updates to
\Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::resolveComponentProps()
and\Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated
- … 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 :) - update the default XB field value so that an article uses Kyle's
- Assigned to tedbow
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@tedbow will be working on #20 once he finishes the issue blocking this one: 📌 Create validation constraint for ComponentTreeStructure Needs work . I just reviewed it, and it's close 👍
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
While Ted starts working on this, 📌 Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components Fixed can happen in parallel. Then this issue updating
\Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated
will automatically result inSdcController::preview()
supporting actual component trees too 👍 - First commit to issue fork.
- Status changed to Needs work
5 months ago 4:35pm 30 July 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Create validation constraint for ComponentTreeStructure Needs work is in!
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@tedbow This should save you a ton of time: 📌 Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components Fixed — I mentioned this in #22.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
… 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 😊
- 🇺🇸United States tedbow Ithaca, NY, USA
- 🇮🇹Italy kopeboy Milan
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.
- 🇫🇷France pdureau Paris
@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
don't seem to work yet, so the component was not fully tested, but the component template doesn't make sense:<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 tje 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), 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.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.Other components
From the example module, with a lot of "todo" annotations, so is it too early to have a look?
- experience_builder_example_components:accordion
- experience_builder_example_components:one_column
- experience_builder_example_components:shoe_tab_group
- experience_builder_example_components:two_column
- experience_builder_example_components:heading
- experience_builder_example_components:image
- experience_builder_example_components:shoe_badge
- experience_builder_example_components:shoe_button
- experience_builder_example_components:shoe_details
- experience_builder_example_components:shoe_icon
- experience_builder_example_components:shoe_tab
- experience_builder_example_components:shoe_tab_panel
- experience_builder_example_components:text
- experience_builder_example_components:video
- 🇺🇸United States tedbow Ithaca, NY, USA
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 examplemodules/experience_builder_example_components/components/compound/side_by_side/side_by_side.component.yml
hasheading: 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 getTypeError: 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 tomodules/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
- 🇺🇸United States tedbow Ithaca, NY, USA
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
- 🇺🇸United States Kristen Pol Santa Cruz, CA, USA
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. - 🇫🇷France pdureau Paris
Hi Kristen,
I understood "shoe" is about https://shoelace.style/ here.
- 🇺🇸United States tedbow Ithaca, NY, USA
@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 . - 🇺🇸United States tedbow Ithaca, NY, USA
-
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 📌 Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components Fixed
-
\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. - I can't get EndToEndDemoIntegrationTest::test to pass. I tried to debug this but can't see a difference in the values
-
re #20 and work @Wim Leers laid out
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
#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?
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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?
- 🇺🇸United States tedbow Ithaca, NY, USA
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
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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:
- I merged in
origin/0.x
to get 📌 Introduce `hook_storable_prop_shape_alter()`, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed Fixed , which introduced some crucial bugfixes tomassageFormValuesTemporaryRemoveThisExclamationExclamationExclamation()
. - That in turn allows reverting the change @tedbow made to the
side-by-side
component to not use ainteger
+enum
. - … 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! - 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... - In #3461499, I added fallback logic to
ComponentPropsForm
to pick the appropriate widget while we await follow-up issues to land (there's explicit@todo
s in place for that). But I did not updateTwoTerribleTextAreasWidget
accordingly. And that's what @tedbow was running into here. Fixed that: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... - 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: callinggetValue()
on aListIntegerItem
actually returns'50'
, not the50
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 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 😊
- I merged in
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Utkarsh extracted #42 points 3–4 to 🐛 Follow-up for #3461499: transform at-rest field storage settings using FieldItemInterface::settingsFromConfigData() RTBC 👍
- 🇺🇸United States Kristen Pol Santa Cruz, CA, USA
🎉 Y’all going to find and fix all the core bugs 😂
- 🇺🇸United States tedbow Ithaca, NY, USA
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 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. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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 updateSdcController::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()
— 📌 Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components Fixed only updatedSdcController::preview()
. You essentially now need to write the inverse ofSdcController::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 📌 Create an Open API spec for the current mock HTTP responses Fixed 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 , that should've been
/xb/node/1
. Fortunately it was clear from context 👍 - visiting
- 🇺🇸United States tedbow Ithaca, NY, USA
@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
- 🇺🇸United States tedbow Ithaca, NY, USA
- Ok. now I think I have handled the problems I mentioned in #45 and @Wim Leers response in #46
-
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 bywrapComponentsForPreview
- 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 🎉
- There are a few more Cypress test failing but I have not had a chance to investigate them
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 reasontree
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
- 🇺🇸United States tedbow Ithaca, NY, USA
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🎉
- 🇺🇸United States tedbow Ithaca, NY, USA
- So my previous JS commits broke UI build gitlab job, so the Cypress tests didn't run
- Fixed that got the UI build gitlab job passing
- The Cypress tests ran again.
- All the cypress tests in a11y.cy.js are now passing 🎉
- The fix for a11y.cy.js also fixed a couple other tests. Went from 7 passing, 4 failing to 9 passing, 2 failing 👍
- 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
- Assigned to wim leers
- Status changed to Needs review
5 months ago 2:58am 7 August 2024 - 🇺🇸United States tedbow Ithaca, NY, USA
- 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.
- 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.
- Fixing the test would go much faster if I could run Cypress tests locally so I will look into that tomorrow.
- 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. - Fixed add-section.cy.js
- Assigned to tedbow
- Status changed to Needs work
5 months ago 11:43am 7 August 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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
- 🤔 The changes made by you after @ctrlADel's last commit (
de8cbb170e95ac6edf0c67dc1c92d008041bba21
) should not remove anything from the original intent, and ideally resolve all@todo
s he added. Looking atgit 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. - 🙏 👆 reveals
$ref
s stuff is not yet updated. But we're lacking specifics for Kyle's intent there: he is using$ref
s but did not specify the corresponding schema 😅. He did not write the correspondingschema.json
file, but he did write the corresponding Twig. Which means you can write the necessary schema, Ted. Seemodules/experience_builder_example_components/components/simple/heading/heading.twig
for example: that reveals there's two inputs Kyle was imagining forheading
:element
andtext
. I assumetext
would betype: string
andelement
would betype: string, enum: [h2, h3, h4, h5, h6]
. You can derive his intent for the other$ref
s in a similar way. - 🙏 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...
- ✅ The changes to the Cypress E2E tests look 👍
- ✅ The changes to
EndToEndDemoIntegrationTest
look 👍
That second bullet is a fair bit of work, but this is getting close! 😊🥳
- 🤔 The changes made by you after @ctrlADel's last commit (
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
- @tedbow Reviewed the MR 🏓
- @finnsky Responded to your MR remarks, please open follow-up issues for those things.
- @pdureau Similarly, but your remark there is a relevant issue already, so commented there: #3446052-21: [PP-1] Create components for a default design system → — and credited you there 😊
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
The Cypress E2E test failures are legitimate — did you know you can see the screenshots it made of failures on GitLab CI? 😊
- 🇺🇸United States tedbow Ithaca, NY, USA
Not sure why
EndToEndDemoIntegrationTest
is failing. Seems that can't a widget "width" property of the two_column componentWill debug more tomorrow
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Not sure why
EndToEndDemoIntegrationTest
is failing. Seems that can't a widget "width" property of the two_column componentThe reason for that: the logic that 📌 Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings Fixed introduced does not resolve
$ref
.Fixed that for you: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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 📌 Move /submodules to /modules Fixed 👍 You'll need to merge that in.
- 🇺🇸United States tedbow Ithaca, NY, USA
@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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
- +1 to your last question: https://git.drupalcode.org/project/experience_builder/-/merge_requests/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. - https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... is still not yet addressed.
- #57 is not addressed, and hence the steps to a working environment documented in
CONTRIBUTING.md
no longer work.
- 🇺🇸United States tedbow Ithaca, NY, USA
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
withoutexperience_builder_example_components
since the example field is now inexperience_builder_example_components
- 🇺🇸United States tedbow Ithaca, NY, USA
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`
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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 part of the issue title — and it's important that that lands very soon. It unblocks a lot of work for 🌱 Milestone 0.1.0: Experience Builder Demo Active . - 🇺🇸United States tedbow Ithaca, NY, USA
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 withoutexperience_builder
The following problems happened when I got rid of the sub-modulea
-
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 ofJsonSchemaDefinitionsStreamwrapper
.field.field.node.article.field_xb_demo.yml
is being saved which depends onexperience_builder.component.experience_builder+two_column
which usescomponents/containers/two_column/two_column.component.yml
. I am not clear whycomponents/image/image.component.yml
does not have the same problem in 0.x. maybe because width is integer and image is object property? - 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 -
This seemed like an easy task but for some reason
- 🇺🇸United States tedbow Ithaca, NY, USA
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- field.field.node.article.field_xb_demo.yml depends on 2 components but only
components/image/image.component.yml
uses a$ref
. - The
components/image/image.component.yml
does not throw an exception because it is being validated on install beforeJsonSchemaDefinitionsStreamwrapper
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 - 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.
- field.field.node.article.field_xb_demo.yml depends on 2 components but only
- 🇺🇸United States tedbow Ithaca, NY, USA
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? - Assigned to wim leers
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@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 ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed 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...
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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: 📌 Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed .
- Issue was unassigned.
- Status changed to RTBC
4 months ago 11:18pm 13 August 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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: — looking forward to that — that'll be critical for the SDC ecosystem! 👏
Follow-up: components left out of this MR
See #68, issue: 📌 Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed .
Follow-up:
name
as the name for an SDC propThis issue/MR includes only a
Component
config entity in its default config (i.e. to make an SDC available in XB) for thecomponents/containers/two_column
SDC. 📌 Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed 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
@todo
s for, because it was a theoretical future, but now it is a reality: 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active .Follow-up: SDC props of
type: object
with no direct matchObvious and common use case, but we didn't have an SDC for this yet → 📌 [later phase] Support `{type: object, …}` prop shapes that require *multiple* field types — also: nested components/component reuse Postponed
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: 📌 Improve the page hierarchy display Active .
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: 🌱 [PP-1] Create components for a default design system Postponed .
… 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 →
committed a8ead8ae on 0.x authored by
ctrlADel →
- Status changed to Fixed
4 months ago 12:46am 14 August 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This caused a missed regression — see #3467972-13: Unable to save node article form — remove obsolete TwoTerribleTextAreasWidget + fix duplicate `XB:image` SDC → .2.
Automatically closed - issue fixed for 2 weeks with no activity.