🇧🇪Belgium @wim leers

It's live: https://www.drupal.org/project/experience_builder/releases/0.2.0-alpha1 →

All follow-ups were followed up … here!

Conflicts now 😭 Too risky for me to rebase at this late hour.

AFAICT this is a duplicate of 🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active . @balintbrews will be able to confirm.

AFAICT this can only be caused by template_preprocess_breadcrumb__as_js_component(), which is something @effulgentsia wrote, so assigning to him — hoping it's a simple fix! 🤞

(That was introduced in #3505993.)

Is this reproducible without using code components aka without <astro-slot>?

Manually tested, works great 👍

I just learned from chat that this being a module instead of config was @longwave's genius idea! Crediting.

Hm, an odd e2e test failure:
cy:command ✘ trigger contextmenu
in copy-and-paste.cy.js. See https://git.drupalcode.org/project/experience_builder/-/jobs/4507208

I don't recall that having random failures.

Re-testing.

Demo mode was added for 🌱 Milestone 0.2.0: the one for Drupal CMS 1.0 Active , but is no longer needed. @lauriii and @effulgentsia confirmed recently that there was no more use for it.

So this issue is basically renaming demoMode to devMode, and using the (clever!) use of a module being installed or not as the boolean, rather than config! 👍

wim leers → made their first commit to this issue’s fork.

Originally reported by @hooroomoo at #3509270-13: Status badge and "Review changes" only update after ~10 seconds → , with a GIF. @longwave couldn't reproduce in #16 over there. Full explanation for why by @larowlan at #3509270-17: Status badge and "Review changes" only update after ~10 seconds → after he paired with @hooroomoo.

Nice work, both of you! 🤘

#13: I had a similar hunch!

Shoot, sorry, @longwave!

Both pieces are in :)

@tedbow: when it's green, can you please mark and assign to @longwave? 🙏 He knows this bit best.

Terrible audio quality, terrible video quality because of d.o's 50 MB limit, and single-take.

But it should adequately walk you through what this does and why.

Per @bnjmnm's approval.

Done. This is the very first code component containing an image ever rendered — forgive the ugliness of my test content 🤣

I'll let @balintbrews record a magnificent screencast instead 😇😜

P.S.: all of this is only possible thanks to the work @longwave and I did on 🐛 Adding the Image component results in a state considered invalid Active 🥳
P.P.S.: @longwave +1'd #25's approach — crediting him.

We ran into the actual real-world need for this: #3507929-25: [Front-end] Allow adding "image" props in the code component editor → .

@balintbrews was hitting

AssertionError: assert($componentSource instanceof UrlRewriteInterface) in assert()

Analyzed it. Conclusion:

1. $default_resolved_value is computed correctly, and is set to the absolute image URL that was specified as the default
2. … but ::getClientSideInfo() doesn’t yet pass that to the client, that’s up to 📌 Split model values into resolved and raw Active to do! 😬
3. so the client cannot send the resolved value to the server, because it simply doesn’t exist on the client
4. … which then hits the UrlPreviewPropSource fallback case in GeneratedFieldExplicitInputUxComponentSourceBase::clientModelToInput() … but that is designed to only be created when there’s a need for actual rewriting
5. I’ll work around it by implementing adding a no-op JsComponent::rewriteExampleUrl() with a @todo pointing to #3493943

wim leers → created an issue.

Well … that totally works 😄🤩

Tagging per #4 + #5.

Per @hooroomoo's review — they manually tested it; it didn’t interfere with any click events for opening the menus which was their concern 🥳

It's already here!

🤩

👍 — but can we add a (tiny) test for this? 🙏

Bingo:

What was needed, is making #attached[import_maps] be respected not just when rendering a full HTML response, but also when rendering each individual component preview.

Paired with @balintbrews — he gave me a hunch!

Note how the example in #10 does not include the import maps!

#6: Good observation, but that's not responsible for the preview 😅

/xb/api/config/component does render preview markup for code components:

  "js.optional": {
    "source": "Code component",
    "metadata": {
      "slots": [
        
      ]
    },
    "field_data": {
      "maybe": {
        "required": false,
        "jsonSchema": {
          "type": "string"
        },
        "sourceType": "static:field_item:string",
        "value": null,
        "expression": "ℹ︎string␟value"
      }
    },
    "dynamic_prop_source_candidates": [
      
    ],
    "transforms": {
      "maybe": {
        "mainProperty": [
          
        ]
      }
    },
    "id": "js.optional",
    "name": "optional",
    "library": "primary_components",
    "category": "@todo",
    "default_markup": "<!-- xb-start-5b3715c6-9e10-4fa9-9692-732bfe366912 --><astro-island uid=\"5b3715c6-9e10-4fa9-9692-732bfe366912\"\n        component-url=\"/sites/default/files/astro-island/pKL_OwLDMlNxnBnBYIsn9BSu349KEpkBmq26qw_z8EQ.js\"\n        component-export=\"default\"\n        renderer-url=\"/modules/contrib/experience_builder/ui/lib/astro-hydration/dist/client.js\"\n        props=\"{}\"\n        ssr=\"\" client=\"only\"\n        opts=\"{&quot;name&quot;:&quot;optional&quot;,&quot;value&quot;:&quot;preact&quot;}\"><script type=\"module\" src=\"/modules/contrib/experience_builder/ui/lib/astro-hydration/dist/client.js\" blocking=\"render\"></script><script type=\"module\" src=\"/sites/default/files/astro-island/pKL_OwLDMlNxnBnBYIsn9BSu349KEpkBmq26qw_z8EQ.js\" blocking=\"render\"></script></astro-island><!-- xb-end-5b3715c6-9e10-4fa9-9692-732bfe366912 -->",
    "css": "<link rel=\"stylesheet\" media=\"all\" href=\"/sites/default/files/css/css_2L-jRULv7_xXe52gIO_bjqU3o7kiFhu1A3RE26luuI8.css?delta=0&amp;language=en&amp;theme=olivero&amp;include=eJxLrShILcpMzUtOjU8qzcxJSS3STywuKcqPzyzOScxL0csvKMnMz0vMAQBNxxA1\" />\n",
    "js_header": "",
    "js_footer": "<script src=\"/sites/default/files/js/js_62jyEDukuEKD8nfERQhtlwDsBadoEVzY0V2toERniis.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=olivero&amp;include=eJxLrShILcpMzUtOjU8qzcxJSS3STywuKcrXy6hMKUosyczPAwDxxA3H\"></script>\n"
  },

Investigating…

Manually tested in the UI, it works:

👍

Tagging because of .

Huh! So I guess I did find the root cause! I actually tried doing exactly what you did here, but couldn't get it to work. Must've been a caching/wrong state problem.

Manually tested (on a fresh install), and it works:

🤩

All 🌱 [Meta] Plan for code components Active must-haves are done! 🤯🥳

Plus, the long-standing 📌 [Needs design] Library confusingly lists SDC-sourced and Block-sourced Components together Active finally landed too, which empowers a much nicer UX for landing

must-haves listed in this meta are done now, so marked each of the 7 requirements as ✅.

Without this, we need to grant anonymous users the administer code components permission for them to be visible 😅

Reflecting that … ALL MUST-HAVES ARE DONE! 🤯🥳

Lovely, thanks! 🙏

Why remove the constraint? Why not instead modify ClientServerConversionTrait and add an explicit test for handling this in ClientServerConversionTraitTest? 🤔

1. New constraint validator that calls ComponentMetadataRequirementsChecker — fails in an interesting way!
2. Realize it A) must only run after JsComponentHasValidSdcMetadataConstraintValidator has verified it works — this is not possible in Drupal's Symfony validation constraint setup. So: merge it.

   Then realize B) that it overlaps with \Drupal\experience_builder\Plugin\Validation\Constraint\IsStorablePropShapeConstraintValidator.

Solution: expand JsComponentHasValidSdcMetadata (+ rename) and remove IsStorablePropShapeConstraintValidator. Less precise validation errors, but simpler code. And it even makes this MR a net reduction in LoC!

💁‍♂️ We can work on making the validation errors precisely target the faulty property path value in the config entity later, if there is a need for that. Because the XB code component editor UI does not even allow creating invalid ones — so only people manually modifying JavaScriptComponent config entity YAML exports and then importing them will ever even see these errors.

P.S.: the Implicit conversion from float 3.14 to int loses precision PHP deprecation warning is due to a core bug: \Drupal\options\Plugin\Field\FieldType\ListItemBase::simplifyAllowedValues() uses the value of each list option as an array key. This works fine for strings and ints (both valid array key indices), but not floats. Try running $test = [3.14 => 'test']; on PHP >=8.1 and you'll see the same. I first thought this was caused by ✨ Enum vales do not have translatable labels Active , but it's not.

Per #3509186-7: Code components: optional properties should not require examples → , this would ensure that examples specified for code components would also be valid. So bumping priority.

This should use ComponentMetadataRequirementsChecker, which will also mean we'll get examples validated in the future automatically, too — thanks to 📌 ComponentMetadataRequirementsChecker::check() should validate that the example(s) actually match the JSON schema Active .

Tests are proving the problem now 👍

Updating title for consistent terminology.

Tagging per https://www.drupal.org/project/experience_builder/issues/3455753#release... 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active .

Updating title for consistent terminology.

Tagging per https://www.drupal.org/project/experience_builder/issues/3455753#release... 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active .

Will test @larowlan's MR after lunch 👍

To ensure we don't forget #8.

Assigning to Bálint for #menu-block-override-js-error in #7, which I bet (and hope) will be trivial for him 😄🤞

Manual testing: getting started

When doing manual testing, I couldn't figure out how to create these from the UI. I was worried that I was doing something wrong. Then I was worried that xb_dev_js_blocks should not have been hidden in the UI (using package: Testing).

But then I discovered the reassuring statement by @balintbrews in the issue summary:

Start with a minimal implementation that doesn't allow creating overrides from the UI, but presents existing ones similar to code components, with reduced functionality. This will allow us to provide an initial set of overrides as config entities (e.g. shipped in a submodule).

All good!

Is it intentional that xb_dev_js_blocks's config is not enforced?

Note: uninstalling and reinstalling xb_dev_js_blocks does not cause the block-overriding code components to be recreated. That means that sites won't get updated config. This seems fine, but I wanted to double-check.

Manual testing: the menu block override has a JS error

I was able to edit the "Breadcrumb" and "Site branding" block overrides in the code component editor, and the preview updated live — awesome!

But for the "Navigation menu" one, the preview didn't render. It showed:

index.js:148 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading '__H')
    at l (index.js:148:19)
    at I (index.js:181:20)
    at B (index.js:168:9)
    at A.NavigationMenu [as constructor] (43d49346-52be-4dcb-aece-2d02a3de425c:5:29)
    at A.ve [as render] (index.js:665:14)
    at z (index.js:241:14)
    at oe (children.js:97:16)
    at z (index.js:267:13)
    at ye (render.js:42:2)
    at code-editor-preview.js:73:3

in the console.

But in @balintbrews's GIF in #5, it did work. 🤔

So I tried to see if there's anything off about this in either JavaScriptComponent::getExperimentalHardcodedBlockProps(), template_preprocess_menu__as_js_component(), or the test values at experimental/js-components-as-block-overrides/example-data/system_menu_block.json.

There's only one problem I found: the label slot. The test values specify it as:

  "slots": {
    "label": "<div>Main navigation</div>"
  }

Whereas the code renders it as:

        aria-label={label}

👆 markup clearly does not belong in there — an ARIA label should be assigned plain text, not markup.

So which is right, which is wrong? 🤔 Looking at template_preprocess_menu__as_js_component(), I find:

  $slots = [
    'label' => $block_variables['label'] ?: $block_variables['configuration']['label'],
  ];

That's clearly a plain-text label.

Which leads me to conclude that the very definition for system_menu_block is wrong — it doesn't have any slots.

Conclusion

I don't know what the original thinking is that led to "label" being a slot for menu blocks, so I defer to @effulgentsia and @balintbrews to address this in a follow-up. Meanwhile this MR brings this functionality alive, so merging with this known bug 👍

It was pretty cool to see my in-browser modified branding block appear on the live site 😄🤩

So you're saying: your MR fixes it with a +13,0 diffstat, barely more than my OpenAPI-only change 🤣

Yeah, that's a no-brainer! Can't wait to test that tomorrow! 😄 (Would be happy to insta-merge after manual testing, and to move what I proposed to a new issue for a distant future.)

🤩

I think reusing the existing ui/src/components/form/components/TextArea.tsx would prepare us better for CKEditor 5 support in the future? See https://git.drupalcode.org/project/experience_builder/-/merge_requests/7....

Looks great!

1. Added missing "invalidate (render) cached block component instances when an overriding code component instance is saved" — which @longwave pointed out: tests (fails) + fix
2. Added missing internal HTTP API test coverage to ensure the client can actually set block_override! Turns out it couldn't 😅 It was missing a one-liner. That'd have been a frustrating discovery for @balintbrews in ✨ [PP-1] Editing code components as overrides, step 1 Postponed !

I think there's plenty of warnings to not risk this ossifying as-is. I agree with @longwave's concerns, but … I mostly admire the very clever solution @effulgentsia demonstrated here 🤯

#4: that works too, but does require an extra request. I know that the polling is still ongoing, and so this would just change the polling rhythm, if you will.

But … #4 is inevitably more concurrent requests immediately after making changes. And hence also more Drupal bootstraps.

I think in my proposal, we can even reduce the polling frequency, to say, every 30 seconds. It'll still feel instantaneous.

Manually tested to make sure it still works when not using a subdir — all good 👍

We missed this in 📌 Code Components should render with their auto-saved state(if any) when rendered in the XB UI Active .

To make this issue's work much more impactful: 📌 Status badge and "Review changes" only update after ~10 seconds Active .

To make this issue's work much more impactful: 📌 Status badge and "Review changes" only update after ~10 seconds Active .

There: I propose to simply make mutations to the layout (POST or PATCH to /xb/api/layout/…) to change:

wim leers → created an issue.

What a rollercoaster this issue has been 😅 @tedbow did one MR, then another. Then @larowlan did a third, and explained why.

Then today, @tedbow and I iterated on @larowlan's third. Seems like @tedbow and I are mostly happy with where things are at, and @tedbow's key refactor was approved by @larowlan, and my subsequent iteration on that was approved by @tedbow, and by @larowlan. He did raise a performance concern, but I think I was able to address that adequately.

@larowlan felt that the test coverage from @tedbow's second MR didn't add much value — and while I see his point, I think there's indirect value for it, so we respectfully disagreed, but @larowlan did indicate he didn't consider it blocking 👍

Finally, 2 follow-ups were identified:

1. @tedbow did discover one thing that's broken, but that is broken in HEAD, too. @larowlan also agreed it could/should be a follow-up. Created: 🐛 Auto-saving of blocks needs to handle string-to-bool fixing Active .
2. @larowlan observed what neither @tedbow nor I observed: that we should be solving this also for those 2 other config entity types. Follow-up created: 📌 Only auto-save JavaScriptComponent/AssetLibrary config entities when there are actual changes Active .

Thanks, everyone! 🚀

wim leers → created an issue.

wim leers → created an issue.

@larowlan just reminded me of this. Still would be very nice to land this.

P.S.: I forgot I even created this. @larowlan talked about #3050881: Use RecursiveExtensionFilterIterator in drupal_phpunit_find_extension_directories for performance → and then told me it's a duplicate of "mine" — this issue 🤣

Letting @larowlan decide when to merge based on https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...

@effulgentsia: see that comment thread, @larowlan was suggesting an optimization that would only work per-frame, and would not pre-load ahead of time. Slightly different than what I suggested on the call earlier :)

Either way: I don't think those optimizations are blocking, but they do merit a follow-up.

🐛 js_component examples and enum do not respect the prop type Active just landed and made this trivial to fix.

All addressed 👍 Some nice catches, @tedbow — thanks!

@balintbrews Thanks for explicitly testing this indeed fixes the consequences in the UI! 🙏👍

FYI: added the (trivial) comprehensive test coverage I wanted: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... — good to go now!

Why? As I wrote in #16: I don't ever want to revisit this again! 😅 →

Once 🐛 js_component examples and enum do not respect the prop type Active lands, XB's code components will actually be at least mostly valid, so they will naturally pass validation here, too.

That's good, because we want to ensure that every valid code component actually works in XB. This issue is about ensuring that SDCs that can actually work in XB, are accepted.

Related problem spaces, but different things!

wim leers → changed the visibility of the branch 3502902-simpler to hidden.

Thanks!

This simpler alternative is definitely easier to land! Especially because @larowlan sprinkled comments on the MR that walk you through what's going on 👍

Still needed:

1. The simpler MR is missing 3 important parts of @tedbow's test coverage expansion
2. @jessebaker feels quite confident we can remove the 11 second timeout — let's try that.

Aha, this is what @larowlan was referring to: https://git.drupalcode.org/project/experience_builder/-/merge_requests/7... 😊

CI agrees! (component-operations.cy.js fails on CI, but passes locally. It's been a known fairly frequent random failure for a while.)

I'd like @tedbow to do final sign-off given he started this MR and I took it in a very different direction.

Also, I bet that @tedbow's 🐛 Javascript code components props, examples and description should be optional Active (which just landed) conflicts with this.