🇧🇪Belgium @Wim Leers

📌 Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active is in, let's do this now!

drush cr alone is insufficient, you need to reinstall (because default config changed). 🙏

IOW: always doing the steps at the top of /CONTRIBUTING.md is your safest bet. 📌 Remove the default value for the XB field and move components to a separate module Active will simplify those steps 👍

The distraction/off-topic "CI allowed to fail" one mentioned in #3 is now fixed: #3472316-11: CI: remove `phpunit.image` override thanks to upstream fix → .

Is this more accurate, @jessebaker?

Oops! See #3472579-3: Tests are sometimes failing → , this is actually introducing CI warnings, so it's a regression. This needs more attention.

I'm confused why it was reported as 100% green here on d.o now 😬

The warning started after 📌 CI: remove `phpunit.image` override thanks to upstream fix Fixed was merged, very suspicious.

Ahhhh … it looks like https://git.drupalcode.org/project/experience_builder/-/pipelines/275317 failed, I merged that prematurely! Reverting…

🐛 Adding a component with slots does not register the slots as children Fixed landed 🚢

Verifying fundamentals

For the experience_builder:heading SDC, the generated Component config entity does contain:

    style:
      field_type: list_string
      field_storage_settings:
        allowed_values:
          -
            value: primary
            label: primary
          -
            value: secondary
            label: secondary
      field_instance_settings: {  }
      field_widget: options_select
      default_value:
        value: primary

IOW: primary is selected as the default automatically, because that's the first example. So why is this not being respected? 🤔

Turns out it is:

So the default aspect mentioned in #12 is an irrelevant distraction.

So: server allows selecting "nothing", but how to make client know this?

All that matters is what @lauriiii was getting at in #10: if an SDC marks a prop as optional, we should be able to detect that (the "no value entered" scenario). So how do we do that?

Turns out that form_select_options() hardcodes a particular special value: _none — specifically for <select>. (Even the "list" field type's widget base class must hardcode this knowledge, see \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase::validateElement()). It can optionally be overridden using #empty_value.

Conclusion

Select.tsx, introduced in 🐛 Prop select lists don't affect the component Fixed , needs to be updated to be aware of this special value. If this special value is selected, then:

1. the UI should interpret this as "no input specified by user"
2. the UI should then delete this prop from the model, rather than setting "_none" as the value — because that's the whole point of ✨ Contextual form values need to be integrated with Redux Active : to allow real-time updates of the preview based on information on the client side only
3. that will then prevent this invalid model to be sent from the client to the server:

That was simpler than expected! 😄

📌 Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active was just fixed.

The two other blockers are in progress too: 🐛 Redux support for ImageWidget: `[image] String value found, but an object is required` Needs work is being worked on by @bnjmnm, 🐛 Adding a component with slots does not register the slots as children Fixed is being worked on by @balintbrews & @jessebaker.

Issue summary updated for you, @kristen pol 👍

📌 Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active landed 🚢

Docs updated. Nits addressed. GIF added to issue summary.

Let's ship this! 🚀

Thanks, @tedbow!

#7: that's 🐛 Redux support for ImageWidget: `[image] String value found, but an object is required` Needs work , not this issue — because you tested using ImageWidget, not MediaLibraryWidget.

This blocks 📌 [PP-2] Add ComponentAuditabilityTest Active .

📌 Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed landed weeks ago.

I think that now it makes sense to wait for 📌 [PP-2] Add ComponentAuditabilityTest Active , which will introduce part of what this aims to address: the beginnings of a UI that allows auditing what components are available in XB or not, and if not: why not.

📌 Mitigate class pollution from injected CSS Needs review is in.

Let's wait for @bnjmnm to reach a conclusion in 🌱 [META] Redux sync on ALL prop types, not just ones with a single [value] property Active first.

So that is why that stupid error re-appeared! 🤪

I knew we fixed it in 📌 Tweak PHPStan rules Fixed , so I did not understand why it re-appeared — I overlooked that path! 🙈

Thank you!

Oops … 🙈

I got this wrong, because I got #3469609 wrong — see #3469609-18: Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs → .

Fixed now.

I do totally believe in you! 😄

Also, I very much messed up the sample commits and issue summary 🙈

Issue summary fixed, and mistake rectified on the MR: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2....

🐛 Adding a component with slots does not show the slots in the layers view Closed: duplicate was marked as a duplicate of 🐛 Adding a component with slots does not register the slots as children Fixed by Jesse.

✨ Allow deleting component instances by pressing "Delete" or "Backspace" Needs review landed 🚢

+1 — I just didn't know you wanted to be involved like that — definitely not back in May! 😅

Another issue that is closely related to this: 🐛 Redux support for ImageWidget: `[image] String value found, but an object is required` Needs work .

I think the solution that 📌 Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Needs work introduced with data-media-file is not scalable/sustainable. But I do think there also is an opportunity to solve this generically.

See https://git.drupalcode.org/issue/experience_builder-3463842/-/tree/34638... for an outline (explained in 3 commits) of what I have in mind 🤓 (Although that surely won't work for real-time updates, only for AJAX updates, which is the case for complex widgets like image, media library, etc.)

Note: AFAICT this will need to call ImageItem::preSave() for it to work, which is called by massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation(). But that is only called upon actually saving an entity, and ever since 🐛 Unable to save node article form — remove obsolete TwoTerribleTextAreasWidget + fix duplicate `XB:image` SDC Fixed , that's not called at all anymore.

I figured out the root cause: this happens when uploading a new image using the ImageWidget.

There simply is no Redux support yet for ImageWidget. That's what 🌱 [META] Redux sync on ALL prop types, not just ones with a single [value] property Active is for.

As of yesterday, 📌 Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Needs work landed and introduced hardcoded special support for MediaLibraryWidget, with experience_builder_preprocess_media_library_item__widget() generating a data-media-file attribute containing the updated field properties.

Phased solution:

1. IMMEDIATE WORK-AROUND: Just install the Media Library module and use the Media Library Widget 👍
2. MR here: introduce similar behavior for ImageWidget, and piggy-back on what #3454173 introduced for Media.
3. Long-term: generic solution in 🌱 [META] Redux sync on ALL prop types, not just ones with a single [value] property Active .

MR incoming.

Tests aren't passing.

🙏 Please don't mark an issue as needing review until the MR passes tests!

#8:

• Possibly. Let's wait for there to be 3 occurrences, to avoid premature abstraction. For now, please add an @see to the other place.
• Yes 🙏

So close! 🤩

Great work here 😊

Figured out the root cause while pairing with @tedbow!

https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... is not yet addressed.

Nor is the rest of #15.

wim leers → created an issue.

AFAICT tests are present and passing now 👍

@jessebaker has pointed out an oversight over at https://git.drupalcode.org/project/experience_builder/-/merge_requests/2..., so marking .

In there, he also points out a scenario that is currently broken and that the test coverage should be expanded for.

Finally: please include a GIF showing this in action.

This must happen after 📌 Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active at least, and possibly more issues, to minimize disruption for in-progress MRs.

Almost ready! 👍

Thanks, @akhil babu! 😄

@lauriii was being trolled by Google Chrome — he literally couldn't type anything anymore 🙃 So, making this change on his behalf 😄

One post-review thought: we need to verify that the state service remains in sync with the SDC discovery cache. Otherwise, we risk that the state key-value pair is deleted while the SDCs already are discovered. That'd result in zero incompatible SDCs getting listed, while we know that's not the case.

Definitely on the right track! 🤩👍

At this stage I'm assuming it's debug css that has been accidentally committed. If it's meant to be there we can add it back into the hero but properly scoped 😄

😄

@jessebaker was just pointing that out!

Reproduced the last failure:

Drupal\Core\Render\Component\Exception\InvalidComponentException: [image.src] Does not match the regex pattern ^(/|http).*\.(png|gif|jpg|jpeg|webp)(\?.*)?(#.*)?$ in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).

Investigating 🕵️

This stopped failing!

• #2885413 was committed on Aug 24: #2885413-157: Timestamp field items are affected by 2038 bug →
• Then a follow-up commit happened on Aug 28: #2885413-164: Timestamp field items are affected by 2038 bug → that changed the behavior
• … and again later that day: #2885413-170: Timestamp field items are affected by 2038 bug →
• … and then it was reverted: #2885413-177: Timestamp field items are affected by 2038 bug → .

So, postponing that on the upstream issue landing.

FYI, per 🐛 [PP-1] Can't toggle boolean prop back to true after changing to false Postponed , @kristen pol created these related issues, which I suspect ought to be child issues of this one:

• 🐛 [PP-1] Can't toggle boolean prop back to true after changing to false Postponed
• 🐛 Redux support for ImageWidget: `[image] String value found, but an object is required` Needs work

I already reported this over at #3463842-6: [META] Redux sync on ALL prop types, not just ones with a single [value] property → , where @bnjmnm is devising next steps. Linking this there.

#5++

Thanks for creating this, @danielveza! 🙏

There is an accumulating overhead for every additional event subscriber. This affects 100% of requests/responses, and if it’s only for architectural purity, then it’s not worth the overhead IMHO. But I don't feel strongly about this, plus at this early stage, that's essentially premature optimization.

So, deferring to the owner of this part of the codebase, @traviscarden, and RTBC'ing 😊👍

👍 Thanks, @balintbrews!

Needs a new round of feedback from @lauriii.

@danielveza That's a relief! 😮‍💨

I'd like to avoid future devs having pain when trying to track down what is most likely a quite annoying performance regression to figure out.

That's totally fair.

Probably the softer approach for XB rather than the feature flag that removes it is a hook_requirements implementation that runs if JOSN:API is also enabled. I'll open a ticket.

I was going to suggest docs but … this is far better! 👏🤩 Thank you! 🙏

Precise STR would help 🙏 Better yet, when you reproduce it, look in the network inspector at the request body for the last request to /api/preview/…. Find the entry for image and you'll see exactly what the value is that is being sent for it. That'll determine where the root cause lies: back or front end.

We can make the page hierarchy (or layers) feel more robust by adjusting the styles in a way where elements are not moving around between state changes. We should consider both focus / active styles, as well as drag and drop.

Is this enough information for you to act on this, @balintbrews or @jessebaker?

First unbroke the MR, by resolving a conflict: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

Then paired with @bnjmnm, to figure out why things were failing in the build for that commit. Turns out that we're hitting a new edge case for input--SOMETHING.html.twig. We discussed and agreed that it'd make more sense for those two files to be omitted and deferred to follow-up issues that are more tightly scoped — they'll be child issues of 🌱 [META] Redux sync on ALL prop types, not just ones with a single [value] property Active .

I trust @bnjmnm's judgment which parts of this MR to land and which parts are more reasonable and feasible to land in a follow-up :)

Great catch/call about uri vs uri-reference, @tedbow!

📌 Also validate request bodies against the OpenAPI spec Downport landed. I don't see how this particular case could occur anymore.

(Not a blocker anymore thanks to @bnjmnm's analysis on #3455975.)

This reminds me conceptually of 📌 Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest() Fixed — I wonder if we can do something similar here? Any inappropriate "wait" would then explicitly fail.

🥳 — thank you, much appreciated, @omkar-pd! 👍

@q0rban: Yay, thank you! 🚀

We currently do this manually when developing XB:

# See it in action + recommended development environment
1. Drupal 11 (preferably a git clone, for git archeology — 10.3 will work too).
2. `composer require drush/drush`
3. `drush si standard`
4. `drush pm:install experience_builder`
5. Browse to `/node/add/article` — you'll see a `🪄 XB Demo ✨` field. Don't touch that — just enter a title for the article and hit save: a component is rendered using the article title 🤓
6. If you're curious: look at the code, step through it with a debugger, and join us!
7. If you want to run *all* tests locally, including the OpenAPI spec one: `composer require league/openapi-psr7-validator webflo/drupal-finder devizzent/cebe-php-openapi --dev`

— /CONTRIBUTING.md

But we also have and end-to-end test that literally performs those steps, added months ago ( ✨ [MR Only] Edit any component prop, powered by a new FieldForComponentSuggester service, which will power the JS UI Fixed ) in the super early XB days, but still serving its purpose: \Drupal\Tests\experience_builder\Functional\EndToEndDemoIntegrationTest::test()

And the Cypress end-to-end tests (similar to Nightwatch, but better) perform similar steps:

    $field_definitions = \Drupal::service('entity_field.manager')->getFieldDefinitions('node', 'article');
    $image_field_sample_value = ImageItem::generateSampleValue($field_definitions['field_hero']);
    $node = Node::create([
      'type' => 'article',
      'title' => 'XB Needs This For The Time Being',
      'field_hero' => $image_field_sample_value,
    ]);
    $node->save();

— \Drupal\Tests\experience_builder\TestSite\XBTestSetup::setup() (@abhisekmazumdar in #4: look at that 😊)

Proposal: if you can make this work to the point where it installs the Standard install profile, creates the first article node (and perhaps fails for now) and then navigates to /node/1, I'm happy to fill in the few missing pieces. 😊😇

🐛 Retain the placement of components within the preview when inserting a component Active landed, let's land this next? 😄

🐛 Retain the placement of components within the preview when inserting a component Active landed!

Is this unblocked now, or is it blocked on 📌 Improve UX of adding new sections Needs review ?

Assigning to @balintbrews to find out 😇

#21:

1. @traviscarden has AFAIK started on 🐛 OpenAPI validation errors must be provided as a JSON response Active 👍
2. I cannot find this follow-up yet among https://www.drupal.org/project/issues/search/experience_builder?issue_ta... →
3. @traviscarden did this: 📌 Add TravisCarden to `CODEOWNERS` for OpenAPI Fixed 🚀

WFM in general, with one exception:

/xb-api/v1/{resources} [ /{resourceId} [ /{subCollections} [ /{resourceId} ] ] ]

-1 to versioning the API at this time, because that implies it's a public API. It is not. Defining a public API for this that we provide BC for is definitely not in scope.

If you really want to have it, then let's use:

/xb-api/v0/{resources} [ /{resourceId} [ /{subCollections} [ /{resourceId} ] ] ]

(with v0 matching XB's major version: 0.x)

P.S.: I do not understand why

We should also avoid /xb/ if we're going to use that path for administrative UI routes.

matters? /xb-api/… vs /xb/api/… are essentially the same?

Lots of MRs landed, this now conflicts.

Fixed issues that should help y'all:

• 🐛 Prop select lists don't affect the component Fixed is in
• 🐛 Component config entities are incomplete: missing entries for optional props, causing errors in ComponentPropsForm Needs work is in ( 📌 Surface the REASON for an SDC not being made available in XB (i.e. not meeting criteria) Active is being worked on 👍
• 📌 Improve the page hierarchy display Active now visualizes the hierarchy better.
• ✨ Allow components to use textarea in favor of input Needs review landed, which means you can now express a shape for your SDC's props to cause a <textarea> to show up — but it won't work just yet — see #3463842-6: [META] Redux sync on ALL prop types, not just ones with a single [value] property →

✨ Implement the concept of sections within the client Fixed landed 🚢

Great! TIL about https://www.drupal.org/project/layout_builder_iframe_modal → 😄

Then the next steps are pretty clear — updated issue summary accordingly.

Assigning to @bnjmnm for review/confirmation of @lauriii's proposal.

Thanks!

Can you please update \Drupal\Tests\experience_builder\TestSite\XBTestSetup to make the same "test with an image that contains a space" changes that https://git.drupalcode.org/project/experience_builder/-/merge_requests/189 introduced for 🐛 `{type: string, format: uri}` disallows image URLs containing spaces, `uri` data type stores + returns invalid URIs! Fixed ? 🙏

That should be able to reproduce it then!

Ideally we could outsource the styling completely to the admin theme so that we don't have to own this within the XB.

I don't see how this is realistic/feasible, because those admin themes will understandably style dialogs on the back-end only, not in the front-end (system.theme:default) theme.

That's an unsolved problem in Drupal core too: #2195695: Admin UIs on the front-end are difficult to theme → .

I don't understand why are we loading the media library within the XB application instead of loading it inside an iframe that would have all the required styles from the admin theme?

No idea — that's the part of #3454173 that @bnjmnm handled, so assigning to him for answering that :) I suspect it's simply "because getting it to work at all was the only goal for #3454173" 🤞

👏🚢

FYI: @f.mazeikis landed 🐛 Component config entities are incomplete: missing entries for optional props, causing errors in ComponentPropsForm Needs work earlier today, he's now pushing this forward 👍

#6.4 WFM!

Do you agree with

Can we get started with a E2E test that fails on failing to find the required HTML attribute of the test_REQUIRED_string prop on the all-props test component? 🙏

?

The back-end side LGTM now :)

AFAICT

_Nice to have:_ Refactor `layoutModelSlice.ts::replaceUUIDsAndUpdateModel()` and `layoutModelSlice.ts::insertMultipleNodes()`, make them unit testable, then write unit tests

is not yet addressed.

That's fine: it's a nice-to-have. Can we extract a follow-up issue for that, and have a less experienced team member take that on? 🙏

@jessebaker reported the same bug at 🐛 It should not be possible to drag a component from one preview into the other Closed: duplicate . Closed that as a duplicate.

This is a duplicate of 🐛 Block dragging components from the desktop view to the mobile view Needs review .

Transferring credit.

👍🙏

📌 Create a component for testing form backend + frontend integration Active landed weeks ago 👍

@bnjmnm, FYI: this has been added to 🌱 Milestone 0.1.0: Experience Builder Demo Active not to solve all the things, but to address at minimum ✨ Allow components to use textarea in favor of input Needs review — and perhaps also type: boolean (the latter is AFAICT a blocker in 📌 Component props form: map textarea, bool, and select elements to React components Fixed being reported by Utkarsh).

(i.e. make at least those 2 field widgets work, just like 🐛 Prop select lists don't affect the component Fixed made <select> work.)

It makes sense to me to prioritize this issue now, so that @bnjmnm can convert it to a meta issue where he outlines his plan for scaling this to all Drupal core field widgets.

With 🐛 Prop select lists don't affect the component Fixed in, I'm now 100% confident this works.

So writing a passing tests should be easy.

One unaddressed nit, and that last commit introduced one more 😅

Merged in upstream, looks like more tests than before are failing.