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:
Side by Side→ postponed to #3467950: Follow-up for #3446722: `side_by_side` + `accordion` (+ maybe `text`) componentsAccordion→ postponed to #3467950: Follow-up for #3446722: `side_by_side` + `accordion` (+ maybe `text`) components- Tabs
This component has two slots for the tabs and the tab items where there is some interdependence between the two which could prove to be challenging for dropzones to handle.
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #70 | after.png | 191.23 KB | wim leers |
| #70 | before.png | 414.53 KB | wim leers |
| #52 | Screenshot 2024-08-07 at 1.29.20 PM.png | 528.34 KB | wim leers |
| #30 | heading-no-field-for-shape.png | 16.89 KB | tedbow |
| #4 | sdc-tabs.png | 10.15 KB | ctrladel |
Issue fork experience_builder-3446722
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:
- experience_builder-3446722
changes, plain diff MR !2
- experience_builder-3446722-2
compare
Comments
Comment #3
wim leers🤩
Comment #4
ctrladelComment #5
wim leersI tried to get this to work but ran into some challenges. I get that there are a bunch of
@todos such as(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:
required a work-around that I think goes against the intent of what you were trying to convey.
The second change (a required
labelprop that AFAICT should've been optional) seems just a minor oversight.But then the third:
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 😅
Comment #6
ctrladelCan 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.
Comment #7
pdureau commentedHi,
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,
ctaTextprop definition is less strict than my-cta'shrefprop 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.imageprop looks like an URL (iri-reference) instead of a plain string.my-button
textprop may be better as a slot, to host any renderable.my-cta
hrefprop :uriJSON schema format is too restrictive (relative-reference & internationalization are not allowed). It is better to useiri-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.textprop may be better as a slot, to host any renderable.Comment #9
ctrladel@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.
Comment #10
nod_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.
Comment #11
pdureau commentedThanks. I would be pleased to have a look.
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.
Comment #12
wim leersI 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 👍😊
Comment #13
wim leersComment #14
wim leersNote: when working on this, I'll also port relevant pieces from #3446052: [later phase] Create components for a default design system, if any.
Comment #15
wim leersBlocked on #3455728: FieldType: Support storing component *trees* instead of *lists* — otherwise the scope of this MR would be far too big.
Comment #16
kristen pol@pdureau
Sounds great! Is this based on this work?
https://git.drupalcode.org/project/ui_patterns/-/tree/2.0.x/modules/ui_p...
Comment #17
pdureau commentedHi Kristen,
This sub-module: ui_patterns/-/tree/2.0.x/modules/ui_patterns_devel
Also, we have a nice submodule for browsing SDC components in library pages: ui_patterns/-/tree/2.0.x/modules/ui_patterns_library
Comment #18
michaellander commentedIs 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.
Comment #19
kristen polThanks @pdureau :)
Comment #20
wim leers#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:
side-by-sidecomponent by default, and has two of the currently existing default components side-by-side (one in each slot)\Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::resolveComponentProps()and\Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated\Drupal\experience_builder\Plugin\DataType\ComponentPropsValuesnor\Drupal\experience_builder\Plugin\DataType\ComponentTreeStructureStrictly speaking, that would be out of scope, but this issue is the one with an existing MR that has
slotsfor its SDCs, so it's the perfect place to do those last pieces of work after #3460856 is done :)Comment #21
wim leers@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 👍
Comment #22
wim leersWhile 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\ComponentTreeHydratedwill automatically result inSdcController::preview()supporting actual component trees too 👍Comment #24
wim leers#3460856: Create validation constraint for ComponentTreeStructure is in!
Comment #25
wim leers@tedbow This should save you a ton of time: #3463986: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components — I mentioned this in #22.
Comment #26
wim leers… 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 😊
Comment #27
tedbowJust merged #3463986: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components
Comment #28
kopeboyI would add at least a default component that has a form input, like an add to cart block or a donate/deposit/withdraw one.
Comment #29
pdureau commentedThanks @ctrlADel for the information. I got a look and here are some feedbacks.
experience_builder:image
$refsresolution don't seem to work yet in Experience Builder, so the component was not fully tested, but the component template is surprising:What is the purpose of this component?
attributesobjectAm I missing something?
my-experience_builder:hero
If there is already a class attribute in
attributes, two class attributes will be printed:It would be better to do:
Adding
attributesin the prop definition is not harmful, but not useful because always automatically added:No slots, is it normal?
heading,subheading,cta1&cta2look like slots.headinghave both "required" and "minLength: 2"experience_builder:my-section
The template doesn't use the expected
attributesobject.The heading prop machine name and label doesn't match. Is it "heading" or "text"?
No slots, is it normal?
textlooks like a slot.texthave 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_componentsmodule.But there are still 41 "todo" annotations. Is it too early to have such a look?
Comment #30
tedbowI 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.ymlhasis " # @todo refs don't seem to work yet" stil right
If I uncomment the "$ref:" line I get
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
if I try to added an article with default values, plus uploading 2 images, for the fields I get
So seems like the expected problem of
modules/experience_builder_example_components/components/compound/side_by_side/side_by_side.component.ymlnot sending any value on tomodules/experience_builder_example_components/components/simple/heading/heading.component.ymlWhen I look at the page output of
EndToEndDemoIntegrationTestwhen fails this is the same.Basicalyy
Comment #31
tedbowTalked 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
Comment #32
kristen polI'm unfamilar with "shoe" design things so I asked chatgpt about it... would love more explanation :)
This is in the summary but I don't know what that means... where are these from? Sorry if this is obvious :)
Comment #33
pdureau commentedHi Kristen,
I understood "shoe" is about https://shoelace.style/ here.
Comment #34
tedbow@Wim Leers re #31 and #20 I think I am going to switch from using
modules/experience_builder_example_components/components/compound/side_by_sideto usingmodules/experience_builder_example_components/components/containers/two_columnthis is simpler example. Side by Side passes object to sub components and two_column simply gives uses 2 slots to put components in .Comment #35
wim leers#34: WFM!
Comment #36
tedbowI changed this to use the Two Column see #34
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
\Drupal\Tests\experience_builder\Functional\EndToEndDemoIntegrationTest::testpasses 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.Comment #37
wim leers#36.2: 🤔 How is that possible?
\Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure::getComponentInstanceUuids()does this:… 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?
Comment #38
wim leersAlso, for some reason these very simple steps that manually reproduce
EndToEndDemoIntegrationTestno longer work:(They're documented at the top of
/CONTRIBUTING.md.)The
node.article.field_xb_demofield 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?
Comment #39
wim leers#36.3: I’ve had to debug that
DefaultConfigTestbefore 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.Comment #40
tedbowThis 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
Comment #41
wim leersComment #42
wim leersI did a high-level review of @tedbow's changes. I didn't like how the SDCs that @ctrlADel built had started deviating significantly.
So:
origin/0.xto 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 tomassageFormValuesTemporaryRemoveThisExclamationExclamationExclamation().side-by-sidecomponent to not use ainteger+enum.50as a choice would always be represented by'50': a string, not an integer!::storageSettingsFromConfigData(), which was added ~10 years ago for addressing precisely this problem. Whew! 😅 Fix: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...ComponentPropsFormto pick the appropriate widget while we await follow-up issues to land (there's explicit@todos in place for that). But I did not updateTwoTerribleTextAreasWidgetaccordingly. And that's what @tedbow was running into here. Fixed that: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...ListIntegerItemand its default widget actually store an string value … 🙃 It's missing the explicit cast it needs: callinggetValue()on aListIntegerItemactually returns'50', not the50its 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.
Comment #43
wim leersUtkarsh extracted #42 points 3–4 to #3465981: Follow-up for #3461499: transform at-rest field storage settings using FieldItemInterface::settingsFromConfigData() 👍
Comment #44
kristen pol🎉 Y’all going to find and fix all the core bugs 😂
Comment #45
tedbowPutting 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::previewis 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::toRenderablewhen I go to node/1 in here
results in the slots being filled and
$renderable_component_tree->getContent()iswhen I go to xb/node/1
Putting debug points in
\Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated::toRenderablewhen I go to xb/node/1 in here
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::renderifydoes not call itself recursively to fill the slots as "slots" is not present.Comment #46
wim leersI 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.) → 🐛
Reproduced.
That's because the UI/client provides this request body:
… because that is what the client received as a response from
/api/layout/node/1.That's why you observe different behavior:
/node/1results in the stored component tree being loaded and hydrated and then rendering correctly/xb/node/1results 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 itIOW: 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 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 #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 , that should've been
/xb/node/1. Fortunately it was clear from context 👍Comment #47
tedbow@Wim Leers thanks for the context
fixed
Comment #48
tedbowSdcController::wrapComponentsForPreviewis 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 bywrapComponentsForPreviewThe 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::buildFormand trying this on 0.x and the MR I think this because on the MR for some reasontreeis 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
Comment #49
tedbowWith 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🎉
Comment #50
tedbowComment #51
tedbowJust 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.@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.
Comment #52
wim leersThanks 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
de8cbb170e95ac6edf0c67dc1c92d008041bba21) should not remove anything from the original intent, and ideally resolve all@todos 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.$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 correspondingschema.jsonfile, 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.twigfor example: that reveals there's two inputs Kyle was imagining forheading:elementandtext. I assumetextwould betype: stringandelementwould betype: string, enum: [h2, h3, h4, h5, h6]. You can derive his intent for the other$refs in a similar way.EndToEndDemoIntegrationTestlook 👍That second bullet is a fair bit of work, but this is getting close! 😊🥳
Comment #53
wim leersComment #54
wim leersThe Cypress E2E test failures are legitimate — did you know you can see the screenshots it made of failures on GitLab CI? 😊
Comment #55
tedbowNot sure why
EndToEndDemoIntegrationTestis failing. Seems that can't a widget "width" property of the two_column componentWill debug more tomorrow
Comment #56
wim leersThe 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...
Comment #57
wim leersIn 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:
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.
Comment #58
tedbow@Wim Leers re
Now the Cypress tests are passing.
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.
Comment #59
wim leersYes, but the difference will be that those modules'
schema.jsonfiles are not being depended upon by the XB module itself. That's what makes this one special.CONTRIBUTING.mdno longer work.Comment #60
tedbowre #59
This is not actually the case anymore. You can install
experience_builderwithoutexperience_builder_example_componentssince the example field is now inexperience_builder_example_componentsComment #61
tedbowre #57
Actually that moved the file to
modules/experience_builder_example_components/config/install/field.field.node.article.field_xb_demo.ymlI 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.jsonSo the update docs in CONTRIBUTING.md work now with
4. `drush pm:install experience_builder experience_builder_example_components`Comment #62
wim leersDiscussed in detail with @tedbow — he'll extract the schema-related challenges in the
side-by-sidecomponent 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 #3454094: Milestone 0.1.0: Experience Builder Demo.Comment #63
tedbowSo 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_builderdependent on the submodule because the submodule can't be installed withoutexperience_builderThe following problems happened when I got rid of the sub-modulea
EndToEndDemoIntegrationTestis 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
JsonSchemaDefinitionsStreamwrapperis 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.ymlis being saved which depends onexperience_builder.component.experience_builder+two_columnwhich usescomponents/containers/two_column/two_column.component.yml. I am not clear whycomponents/image/image.component.ymldoes not have the same problem in 0.x. maybe because width is integer and image is object property?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.
Comment #64
tedbowok I figured out why 0.x does not have the problem that this MR has with
EndToEndDemoIntegrationTestthat I describe in #63.2. This is because in 0.xcomponents/image/image.component.ymluses a$ref.components/image/image.component.ymldoes not throw an exception because it is being validated on install beforeJsonSchemaDefinitionsStreamwrapperbeing used but because the it uses a dynamic source type it will not throw an exception that we ignore because of the next pointValidComponentTreeConstraintValidator.php:81so the last line will lead to the validate trying to use the
$refBut 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
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 problemsFWIW this problem does not happen if I install the module manually
Comment #65
tedbow#64
I didn't know that
\Drupal\Core\Config\Development\ConfigSchemaCheckertook 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?Comment #66
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 😅
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...
Comment #68
wim leersFirst: 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_sidecomponent 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
textcomponent 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.
Comment #70
wim leersIssue 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: #3467950: Follow-up for #3446722: `side_by_side` + `accordion` (+ maybe `text`) components.
Follow-up:
nameas the name for an SDC propI 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: objectwith no direct matchObvious 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:
Comment #72
wim leersComment #73
wim leersThis caused a missed regression — see #3467972-13: Unable to save node article form — remove obsolete TwoTerribleTextAreasWidget + fix duplicate `XB:image` SDC.2.