🇺🇸United States @bnjmnm

I have this solved and will soon have tests for it in my in-progress MR for 🌱 [META] Redux sync on ALL prop types, not just ones with a single [value] property Active . If this is crazy urgent and can't wait on the larger issue feel free to reopen and assign this to me and I'll extract what I have in there.

In 🌱 [META] Redux sync on ALL prop types, not just ones with a single [value] property Active I am working on a holistic solution that will likely address what is reported here + expanding support for many previously unsupported prop/field types. What was reported here requires functionality that hasn't been built yet - but it is actively being worked on.

Do we need the iframed media library to modify the markup directly? Couldn't we do this in a more structured way where we tell the media library to call a function that returns what was selected in the media library? Then in the XB app we translate that into how the media library form element / widget should be updated.

This is possible, but it should be taken into consideration that the dev resources necessary for this approach this are likely nontrivial. If there are time-consuming surprises similar to the initial ML implementation, this approach might take 1-2 sprints to complete and would have to be done by someone with a skillset >= staff engineer. There's a good chance doing this approach would noticeably slow down other efforts, but perhaps that tradeoff is warranted. While I can't really make that call I at least want to provide the info to help the call be made confidently.

It's not much on its own, but opens up all sorts of excellent followups.

Does that make this more feasible?

Unfortunately no, I was not using the correct terminology. I will edit my comment to make it clear I'm referencing the Media Library modal, not the widget in the entity form

I'm not sure how feasible iframing the media library widget is, but I also don't want to dismiss something entirely until I've had a chance to dig further.

Layout builder's architecture seems more welcoming to this kind of solution
In layout builder:

1. edit form opens in a dialog or frame
2. submit the edit form and the changes are saved server side
3. The entire #layout-builder is rebuilt based on the updated server side values saved in the prior step. The methods in \Drupal\layout_builder\Controller\LayoutRebuildTrait

The Media Library widget, otoh, does not work like this. It isn't updating anything in the DB that will then be available after a refresh. Instead, it is updating the DOM directly. Specifically, it is updating fields in the entity form with values that will not be represented in the back end until the entity is saved.

So to make this work I believe it would be necessary to create a translation layer that allows the iframed Media library widget to update the DOM of the main document. We know that is possible because the DnD layout does exactly that. Given the implementation hurdles of the initial Media Library implementation, I suspect implementing this would be more time consuming & complex than the frame <-> document communication used for preview.

I'm happy to find out I'm wrong about the anticipated complexity - but if it turns out I'm correct it may be worth evaluating if doing it the iframe way is important enough to devote 1-2 sprints to implementing.

I certainly ran into hints of this in 📌 HTTP API: update /xb-component/{component_id} to list possible prop sources for current entity context Fixed as I had to fix test failures related to /xb-components now completing after the layout preview.

The difference wasn't noticeable enough on my machine to have me concerned, but I suspected this endpoint would eventually get bogged down. Fortunately, AFAIK, this is very cacheable - the response content only changes if an XB-SDC is added/removed/edited, and if the cache is repopulated when SDCs change then there wouldn't even be slowness on the initial request.

Good stuff! Merged.

Looks good - committed.

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

Svelte part looks solid and this is a nice step towards the presentation layer not making decisions about what a source does.

@balintbrews I have only one bit that needs looking at - could you either remove the style change in the (i think) unused class or explain where it is used?
After that just switch back to RTBC and I'll commit.

Brute force Stark as a dependency is fine.

Reviewed FE only and approved as the FE person in the MR.

Based on the extensive debugging it looks like this can be addressed by changing a few lines so components (a list of the available components) is no longer a dependency of the iframe-impacting useEffect or the updateData useCallback. Neither of these should need to update if the components list changes.

While it's true that updateData requires components to have initialized for it to properly add a component to the layout, this doesn't need to be enforced as a callback dependency because the UI doesn't offer that option until it is ready anyways.

I think this solution is sound but it would be good to get another FE person to look it over to ensure this isn't going to break something elsewhere.

Further debugging shows that this MR is not creating any new changes that trigger the iFrame attribute update changes. Instead, it is changing the order that they change, and components is changing after layout and model.

A change to component triggers the useEffect that sets data-test-xb-content-initialized to false, but unlike layout/model it does not result in the iframe loading anything new, so the onload that would set data-test-xb-content-initialized to TRUE is not invoked.

So the widget works in the prop form and can very much update a component, but getting this process to work well in E2E proven a bit tricky.
@Wim Leers said he'd have a look at this so assigning to him. Do a grep for @yo-wim and you'll find explanations next to their relevant lines of (sometimes commented) code.

There's a remaining test failing because it is failing to find an iframe where data-test-xb-content-initialized is TRUE and I discovered why by adding console.log() calls next to where data-test-xb-content-initialized is switched to false and true.

The two final logs - both which change the attribute to FALSE (without the expected switch to TRUE) - only happen in this MR.

I can see how this might happen since the switch to FALSE will occur any time the useEffect is triggered by a change to dispatch, height, handleDragAdd, handleDragEnd, handleDragStart, layout, model, components, but the switch to TRUE only happens when the onload callback is invoked. It seems like there are plenty of opportunities for one of the 8 useEffect dependencies to change and change data-test-xb-content-initialized to FALSE without onload firing to change it to TRUE.

If those extra renders of the iframe are unnecessary, perhaps they should be addressed in this issue. If it is expected and acceptable, then we need a different way to mark something as initialized.

WITH THIS MR OPEN YOU CAN CONFIRM THIS BY... going to the XB UI and look at the iframe html - you'll see the data-test-xb-content-initialized within moments of the UI loading.

Re #58

Do we need to write this much custom CSS for this? Can we not use the properties that are provided by Radix Themes

I think we should be using the Radix properties wherever possible and I don't believe there's anything preventing this on a technical level.

bnjmnm → created an issue.

Right now the MR is presenting boolean inputs with a Switch component. The screenshot in the issue summary does not show a Switch. Can either the issue summary or MR be updated to reflect what we should be working on - whichever one is the actual intended goal.

This seems good to go though if possible I recommend waiting on ✨ Implement simplified zoom interface Fixed to land then refactoring this a bit as the other way around might be more difficult and possibly require an additional issue.

Re #47

Using these toggles does not update the preview

They were updating the preview until this commit. which changes the native <input> to a Radix component. I suspect it's a matter of re-shaping the props/attributes to whatever structure Radix needs. In particular there's an onChange callback in the attributes object that is necessary for syncing UI. It looks like this would need to be assigned to <Switch.Thumb> based on a quick scan of the docs

Can these be made to sit side by side with the label on the left and the toggle switch on the right( I need help with this. I tried styling using css(inline-flex styling) but that is not working.This is wrapped around the inputBehaviours which is causing it to appear it this way.)

This is not related to inputBehaviors, nothing there is presentational. Like twig, form elements are wrapped with a form element template - in this case FormElement. Also like Twig, this template also determines if/where labels are displayed (note the titleDisplay).

With that established, you have several choices for how the input/label relationship is set up for the <Toggle>.

• Add conditionals to FormElement that add styles to the wrappers to display the label/input next to each other and update the label position option so the label appears first
• Set all these properties in a preprocessor, before it even gets to the front end
• Configure it (either via preprocessor or FormElement so the label doesn't display there and pass that responsibility to the <Toggle> component. and consult the Radix docs for how to build this more granularly.

The MR is messy as heck right now, but the widget now updates the layout: https://youtu.be/DFiE4r5mEFE, though as discussed previously it is passing the file URI so we're not seeing a new image there, but the alt text change and the prior image going away is pretty good evidence that it works.

There's and extra step to get the existing image to show up in the media library widget when the props form opens, because it's not actually a media entity. Rather, it's an image field using the media library widget. If we want to do this, it will need to create a media entity for the image (perhaps on the fly) so it can be displayed within the widget. That should be easy enough, but I may ask around to be sure it's necessary.

The MR probably has some junk in there from the different approaches I tried to make it work but I assure you it'll be less chaotic soon (but probably still little chaotic)

Wim Leers → credited bnjmnm → .

Looking good - got some nits and bits in the MR that I'm sure can be addressed pretty easily.

Looks like bool/toggle wasn't blocked on anything back-end, so I just added it to the SDC all props component

And the fix in #34 got the select to work without a crash.

So with that, the 3462310-scaffold-contextual-form-inputs branch has working versions of all the template types. Now we pass it to @Utkarsh_33 to make them look good + clean up any CS violations I might have done when setting that up.

Ah, so this is accounting for non-react JS execution that might still be in-progress after the dom element is spotted in the iframe. That was not accounted for previously and it is happening so it's nice to have a non-ambiguous cause.

This seems like something that would be a challenge with any e2e test on this type of application - not just Cypress. Excellent work narrowing it down to a specific cause vs stuffing the files full of cautiously optimistic waiting. Approved the MR and looks like it just needs 1 more codeowner.

Here's a rewording of #31 with the questions a little more explicit

• I'm adding a few things temporarily with form_alter so it is possible to build the components even if there aren't props widgets for them yet. I'd like to provide something so toggle could be worked on - what is something I could add to a $form array that would best resemble what will be rendered once a boolean prop is available in the all props component?
• For the enum/select, the select options are not making it to $form array, formTemporaryRemoveThisExclamationExclamationExclamation provides a select widget with only none as an option even though the all-props component has options. I'm sure I can get that fixed but wanted to first check if anyone here would immediately know where to go before I start digging.

Very nice @balintbrews! While my original solution addressed the symptom, it felt more like a hack than an actual improvement / fix. Your solution, on the other hand, addresses the underlying cause and removes some debt in the process.

Added the 3462310-scaffold-contextual-form-inputs MR - it's gonna fail CI for sure, but has textarea, Uri(Link) and Select Working (though note the ComponentsPropForm on the back end is not currently getting options for the select so I'll need to poke around. The regular text field has always worked, so all that's left is Boolean, and I'd like to discuss with some other backend folks about how to best implement

@Utkarsh_33 you are welcome to either merge 3462310-scaffold-contextual-form-inputs into your branch or just work directly on it. I'll be making changes to it as well but we're focusing on different parts, and I'm fine fixing merge conflicts if need be.

This is now only a problem when Media Library is enabled -- I'm working on getting the widget integrated in 📌 Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Needs work so I'll consider this in the scope of that issue.

The Link in the screenshot is of a single URI field. A link field in drupal is a two textfields -> combination of URI + The link text. Can the requirements be updated to either provide a text+uri link widget example, or clarify the expectations as being a URI field? If it's the former, we'll need to provide a component with a link field, in addition to the string in URI format that is currently there.

For Toggle, I can wire this up and test with a temporarily added field to confirm it works, but since there isn't (as far as I know) a component that has this yet you'll just need to believe me. This is similar to the <Textarea> obstacle mentioned earlier.

I'll take care of the initial scaffolding for this then unassign myself. Out of the ones listed, input and select are fairly straightforward but the other 3 could get frustrating without some initial setup - I'll do that initial setup.

This feature does not exist yet, and there is an existing issue to add it ✨ Include component props form in undo Needs review

bnjmnm → created an issue.

This isn't quite what I suggested in #31. Left a comment in the MR

Also will need tests - #31 also has an approach to how.

MR fixes this by effectively closing the panel between selections, though to a civilian nothing is different.

Needs tests, though.

This can also happen if there are JS errors - what is the console telling you when this happens?

bnjmnm → created an issue.

With the changes I just pushed up, the widget now works in the props form -> Short video if interested

It does not yet update the layout when the media is updated, though, so that's the next step.

1) So we have only a few form elements which have a custom(created by us) jsx component ex:- textArea and input. So do we need to create all the custom for elements just like the one created in this MR for Select(not completed robust though)?

Yes, create elements.

2)If we have to create custom elements as asked in point 1 then would just mapping the components in twigToJSXComponentMap load them in the DummyPropsEditForm.

For any new components added, you also need to add a .twig file in experience_builder/templates/form that defines the data types variables passed to the template. These are the same variables passed to a regular twig template, and defining the data types lets us know if it's a variable that should immediately react-rendered or if it should be passed as a prop.

Look at the existing files there to see the naming conventions and file contents should be and you'll likely have a good sense of what is needed. Get as far as you can and leave specific/targeted questions here and I'll get them answered asap.

I Improved the test result output for this to state what is being looked for in <head> and, on failure, what is actually in <head> and it doesn't look like the base.css is in there.

 AssertionError: Tried to find [href*="/core/themes/olivero/css/base/base.css"] in <head> <link rel="stylesheet" media="all" href="/web/core/../modules/custom/experience_builder/components/my-hero/my-hero.css?si5uut">
<style>.preview-dragging .sortable-list{
  min-height: 2rem;
}
.preview-dragging .sortable-list:empty{
  background: #fff;
}
.slot-container {
  display: flex;
  > .sortable-item {
    width: 100%;
  }
}
.sortable-list:empty {
  min-height: 1.5rem;
  border: 2px dashed #ccc;
  position: relative;
}
.sortable-list:empty:after {
  content: 'Slot';
  top: 0;
  right: 0;
  text-align: right;
  position: absolute;
}
.sortable-item {
  cursor: grab;
}
.preview-dragging *,
.preview-dragging {
  cursor: grabbing;
}
.sortable-ghost {
  opacity: 0.5;
  padding: 0;
  height: 2rem;
  max-height: 2rem;
  width: 100%;
  margin: 0;
  clear: none;
  position: relative;
  background: linear-gradient(135deg, #DDD 12.50%, transparent 12.50%, transparent 50%, #DDD 50%, #DDD 62.50%, transparent 62.50%, transparent 100%) center / 5.66px 5.66px;
  outline: 1px solid #ccc;
  box-shadow: none;
  flex-grow: 1;
  overflow: hidden;
}
.sortable-ghost * {
  visibility: hidden;
}
</style>
: expected null to exist

bnjmnm → created an issue.

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

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

Good call on doing a round of cleanup when it's still a manageable scope.

Wim Leers → credited bnjmnm → .

Found out what is causing the pile of olivero assets being added

• core/drupal.ajax depends on core/drupal.message
• Olivero extends core/drupal.message with olivero/drupal.message
• olivero/drupal.message depends on olivero/messages
• olivero/messages lists olivero/global-styling as a dependency, adding ~45 CSS files and the olivero/navigation-base library. Seems like a bit much for a messages library?

The issue I'm running into when attempting to add media via the widget might be the same as the one in #31, but when I discussed it earlier I was showing where it fails server side - it is it looking for an 'image' field on the node entity vs what I'm guessing should instead be a property of a ComponentTreeItem?

If press play in the debugger and let the PHP take its course the end result is the same as#31.

It's not the slickest, but here's a few screenshots of the trace, starting with the most recent just-about-to-throw and a few steps back.

Nice to see this happening. I had a suggestion for applying the formatting more broadly to the tests, and once that is OK I'll probably click approve.

What do you

mean? Server responses? A race condition bug in the client/UI?

Race condition - it is looking for offsetHeight on iframe contents and occasionally that property does not yet exist. By having it handle that gracefully, it stops the error and the code simply executes again when the contents are ready & with the expected offsetHeight, this additional execution also happens more quickly than any human could detect.

The MR is-undrafted and adds

• the graceful offsetHeight mentioned aboveabove
• handles the uuid tracking the the router test more effectively - I should have remembered that the original approach is a Cypress DO NOT DO THIS.
• Installed and set upcypress-terminal-report which provides richer console output when a Cypress test fails

Two out of the three fails mentioned in #15 are failing due to application errors, not anything that can be mitigated by changing the test. I think it would be reasonable to include fixing that underlying instability in this issue. First commit won't have anything for this but I'll have something shortly.

The other failure appears to be due to a click inside a component not making it to the event listener that would trigger the process of opening the contextual form. The request to get the form contents from the back end is never made, so network wait would not address this specific problem. One thing I do notice is that the element missing the click is not in the viewport. While this isn't necessary for Cypress (clearly this is passing the majority of the time) I wonder if out-of-viewport + iframe is resulting in occasional bad coordinates. The first commit in the MR is changing the elements in the test to be in-viewport ones and lets see if that helps.

Wim Leers → credited bnjmnm → .

I tried this out and it works great. If anything is missing NBD - we just add to it later - as-is this unlocks a bunch of things we've been aiming to do.

Postponing - I'm going to investigate a little bit more to rule out a few things before anyone should work on this.

bnjmnm → created an issue.

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

Comparing the composer output of a failing job vs one the works strongly suggests that making this path dynamic will address the intermittent line 354: cd: modules/custom/experience_builder: No such file or directory error that fails jobs.

bnjmnm → created an issue.

Any idea how to fix this?

I would not expect anyone to conclude this from the failure output, but getting the branch current with 0.x typically takes care of this. I just did this and pushed so this is back to passing.

I added the 3458535-test-gitlab-ci branch to help narrow down the Cypress tests CI job dying before Cypress can even begin. Turns out I didn't have to do anyting other than possibly getting it current with 0.x - if it's easiest to continue work on that branch that's fine - the only difference is it is current.

Tagging with needs tests. This should be testable since cypress-real-events supports middle-clicks.

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

Starting from scratch gets the component to install and I can see it in the XB UI and it does its job nicely

The problem I ran into earlier (or something similar to it) seems to be happening still - the edit/add form for this SDC isn't working. This is less urgent since it installs with the module now, but here's what I'm running into when I try to edit in admin/structure/component/edit/sdc_test_all_props:

If I try to edit the all-props component when I click submit on the form, the submission is actually blocked by JS and there are console errors: An invalid form control with name='sdc_test_all_props+all-props[<<daterange field|UUID field>>][field_type]' is not focusable. ​. It was reasonably easy to conclude this is due to the date range and UUID props not having field types selected - but still required more sleuthing than I'd expect a normal user to do (I could see this being out of scope here since it is a test-only component)

Setting the field types in the form allowed me to submit the form, but I got the error

Error: Call to a member function massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation() on null in  
                                          Drupal\experience_builder\Form\ComponentEditForm->copyFormValuesToEntity() (line 317 of                              
                                          /Users/ben.mullins/Sites/drupal/modules/custom/experience_builder/src/Form/ComponentEditForm.php).    

Since this is test only and VERY helpful to have, I think it's reasonable to address the edit form issues in another issue. For the time being, I recommend adding something specific to the edit form for this component that warns about this and perhaps makes the edit fields unavailable "This test-only component is provided as-is and cannot be edited" so we don't run into contributors losing their (or our) time assuming this would work.

This will 100% need a dependency on the media module since we're assuming it is there, and even though it's not necessary to function the media library module should probably also be there since that's the widget we indend to use.

I can't add this as a Page Component, but I suspect it will be easy to address as the error specifies a line that was committed only yesterday

 Warning: Undefined array key "#markup" in Drupal\experience_builder\Controller\SdcController->preview() (line 370 of                                            
                                          /Users/ben.mullins/Sites/drupal/modules/custom/experience_builder/src/Controller/SdcController.php) #0                                                          
                                          /Users/ben.mullins/Sites/drupal/core/includes/bootstrap.inc(108):

FE looks good

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

bnjmnm → created an issue.

There are 22 files being changed in the MR and the majority don't appear to be related at all to the reported error. Lets either clean that up or create a new MR.

All set.

bnjmnm → created an issue.

Left feedback on the MR

Add component button is supported on the frontend side. But currently we don't have testable nested component/slots yet so there's no way to test it besides with the hard-coded layout-default.json file.

It should be possible to use an intercept in Cypress to have a test that uses the layout-default.json file instead of the backend data. If that isn't possible, explaining why in a comment will be helpful.

This needs tests as well. In Cypress Intercepting requests and returning junk should make it possible to trigger some errors for testing.

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

We need a test that would have caught this problem in the first place so we can ensure this fix remains in place.

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

This is a good idea! Could the method live in utils and be exported? This way we can use the same method anywhere it's needed, and once we've refactored everything away lint or IDE will let us know it's unused.

I got FE and BE signoffs, so here's yalls previews.
Committed.

The issue summary mentions both "add section" and "add component" (for slots). I do not see the latter here - that should either be addressed int he MR or given its own issue (the additional issue might be more manageable IMO)

s per your comment #17 I didn't apply any changes on vertical tab. Instead of it I have removed extra margin from the top and bottom of "Create new revision" Checkbox field. So I think it will not affect if more than one tab is present. I have attached the screenshot "top-bottom-margin-removed.png" for reference.

This new merge request 8888 has essentially the same problem but slightly worse
In the MR I had concerns with in #17, it was because it was disrupting/breaking the styles of all vertical tabs to address the odd appearance when only one tab is present.

This newer MR is disrupting the styles of all <details> these include the ones used to create vertical tabs but also every details element on the site.

/* Details content wrapper */
.olivero-details__wrapper {
  margin: var(--sp1);

  @media (--lg) {
    margin-block-start: var(--sp1-5);
    margin-block-end: var(--sp1-5);
    margin-inline-start: var(--sp2);
    margin-inline-end: var(--sp2);
  }

In addition, the style changes of both these MRs only work if the tab is only one line long, it's not actually fixing the issue, it's just masking it for the specific content length and viewport width you're working with.

As mentioned earlier, an approach that styles single-tab vertical tabs differently is your best bet.