🇺🇸United States @bnjmnm

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

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

Looks like 📌 Add support for global regions Active replicated a ton of this and it's probably easiest to start fresh with a new issue instead of gumming up the current issue fork. Ideally that issue will have a link to designs .

bnjmnm → created an issue.

I'm not sure what to triage past the metadata that's already been set on the issue.
However, it's a simple enough change that there's an MR to take care of this. We could add a bunch of "needs design"-esqe tags to do this thoroughly but I'm partial to getting this out there and allowing the response to these changes inform any changes to the implementation.

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

EVERYTHING BELOW IS NOT OCCURRING WHEN I RUN E2E TESTS, SO I'M RELUCTANT TO EVEN MENTION IT But this is a big enough structural change that I'm going to document this in case it surfaces something legit. Perhaps something in the e2e test setup is addressing the underlying cause?

With this branch I see the following as soon as I load any article I visit in XB, new or existing. I also checked on a new Drupal install / different browsers to rule out autosave data messing with things. The screenshot is for the TextField component but I also run into it which other components.
Warning: `value` prop on `input` should not be null. Consider using an empty string to clear the component or `undefined` for uncontrolled components. Error Component Stack... (see screenshot → for the whole darn stack)

If I pull in a new SDC, the form is not populated with the default values

Unassigning myself for now, and I'll see if anyone is around to check this on their local.

Left feedback in MR

aaarrrggg

To my surprise, 0.x is green now?!

0.x still has an arbitrary wait that intermittently allows the tests to pass.

Three things were addressed:
After these two changes, tests started passing ~50% of the time instead of < 5%

1. At first, it was always the first of the two iterations failing. Debugging this narrowed this down to the extra time required to load the page data form in the right panel, which in turn often delayed the preview ready past the default wait. Additional test runs would pass because the assets loaded by the form were cached and thus everything loaded quicker. This was fixed by adding a wait for an element within the page data form immediately after loading the XB UI
2. Changed it so an assertion that requires going into the iframe occurs after a call to waitForElementContentInIframe because the repeat in waitForElementContentInIframe will repeat in a way that accounts for the iframe possibly being swapped, thus avoiding problems of the assertion being stuck inside a no-longer-active iframe.

After the above, tests were passing maybe half the time which made it difficult to know what changes were impacting things. The one consistent thing in all the trial errors was the failures were always occurring with the second test regardless of what was being tested. No matter which test came first, it was always the second that would fail. For this, I turned the forEach into two separate tests and moved the Drupal install to beforeEach instead of before. At least as of the moment I'm typing this, making this change has resulted in the tests reliably passing. I will run a few more repeats to be sure and will weep if it continues to fail.

🤦🤦🤦🤦🤦 This is proving challenging! As established earlier, adding a wait just after visiting the XB UI will reliably get the tests to pass.

With further debugging, I've confirmed the failing tests find the a Dropzone, but the dropzone onAdd never fires. I think this now means narrowing down what about the wait allows the dropzone to accept drops. This shall continue.

Documenting where I'm at.

There are three test cases: ['xb/node/2', 'xb/xb_page/2', 'xb/node/3']
It will fail on the first of the three cases, regardless of order

It can pass if an arbitrary wait is added to the test right after the first step of calling cy.loadURLandWaitForXBLoaded({ url: testCase }); Here is a run where that wait is added + and has a bunch of things logged - all request times, the # and duration between re-renders of several components, and the time it takes for src to load in the preview iframes. Enough is logged that it's a bit intense, but I suspect the info we need is in there.
https://git.drupalcode.org/project/experience_builder/-/jobs/3652460

Removes the wait, fails on xb/node/2 (the first), has all the same things logged
https://git.drupalcode.org/project/experience_builder/-/jobs/3652423

Removes the wait, fails on xb/xb_page/2' (the first), has all the same things logged
https://git.drupalcode.org/project/experience_builder/-/jobs/3652698

Since it is only the first run that fails AND it is fixed with an arbitrary wait at the beginning of the test, then one possibility is slowness from a resource that is then cached thus loading quick enough for later test runs Each request is logged with duration so I’ll dig into that more.

bnjmnm → created an issue.

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

Yikes that was a long time spent on one tiny e2e test.

I tracked this down to the ComponentOverlay component that sit on top layout nodes in the iframe that receives the click to trigger opening of its edit form. When its a block in the layout, the corresponding ComponentOverlay briefly lacks the correct click handlers to. It then re-renders and it works. The longest it has taken to rerender in my test runs is 28ms,

I added a line to e2e test that effectively waits for this rerender

   cy.get('#cea4c5b3-7921-4c6f-b388-da921bd1496d-name').should((blockName) => {
      expect(blockName).to.have.text('Administration');
    });

During the initial not-working render, the text of #cea4c5b3-7921-4c6f-b388-da921bd1496d-name is just component. Once it's Administration it means the click handlers are working too.

Obviously we shouldn't have component overlays that don't work until a 2nd render, but I'm not 100% sure this is something caused by any of the changes here - this are of the FE wasn't really touched. If there's no evidence the changes here are introducing the problem I'd recommend landing this so we can progress with block integration and the broken-for-a-few-ms ComponentOverlay can be tackled in another issue.

e2e is finally passing, but only via adding an arbitrary cy.wait() before clicking the Admin menu block in the the layout to open its block settings form. Without this wait, clicking on the block does nothing. Clearly there's a process that needs to complete before clicking the block, but it's not yet clear what that is. Will keep digging.

I added a

3491013-but-with-ts-muted

branch which (as of rn at least) is the same a the primary MR, but with a bunch of ts-ignores so e2e tests can run and help us identify other places the FE code needs to change. I had hoped to find a few places to fix in the process but aside from fixing a few ts bits (and avoiding a few additional ts-ignores), none of the fixes were particularly obvious (@jessebaker has said as much). Hope the e2e-running branch at least helps things a bit.

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

Re #37 👏 I'd been hoping we could get to a point where the form could just POST what it has vs a bunch of client side manipulation and this seems like a feasible way to do it.

Some things that come to mind that may not need to be considered for this initial implementation (or at all 🤷) but just to get them out there:

• Are there scenarios where validation status is based on something more complex than what can be easily enforced clientside (html5/AJV)? Particularly if the criteria can't be determined until the value massaging takes place? If this is the case, perhaps this can be factored into what the Preview Endpoint returns - if the FE is aware of a validation issue it can then output a message etc and it will be clear why the preview is not updating.
• This approach seems useful for other component edit forms such as block settings, which would also presumably POST to the preview endpoint. Although I think everyone participating in this issue is aware, I want to explicitly mention that the props form will work differently from forms such as Block Settings or Entity Forms because although SDC Prop Forms use field widgets, they (unlike those other forms) are not working with fields and have different mappings, validation etc. I believe the data needed to account for those differences is already present in the proposed approach, but JIC.
• I've been wanting an excuse to work with parts of the Redux API likeonQueryStarted + updateQueryDate, but having not used them yet I'm not sure what specifically is being addressed through their use instead of a plain old store update. I have my hunches, but if you can share details I don't have to make a wrong assumption in public

RTBC'd and code-approved. Needs some other codeownder signoffs in the MR before it can be merged.

One small change requested in the MR then I think it is good to go.

The MR has a fix and a FunctionalJavascript test that verifies it works. Using FunctionalJavascript instead of Cypress E2E allows us to directly test file uploads.

However, as someone not all that well versed in Gitlab CI, I'm having trouble getting the built UI app to be available when the FunctionalJavascript tests run. Anyone is welcome to jump in and address that, and if this sits for too long I'll hit up the Gitlab docs and take care of the rest.

balintbrews → credited bnjmnm → .

I checked a few spots with table cells that contain .form-item and I saw that in taxonomy term tables the "Status" items are no longer aligned with their header.

This is due to a change introduced in 🐛 XBEndpointRenderer adds response headers that sometimes exceed common server limits Active , it'll be a quick fix + a little additional time to add tests that prevent this from regressing.

Other people on this issue are making compelling arguments to go back to the JSON response. I switched it out yesterday because I thought it effectively preserved the error handling but it looks like that isn't the case. The motivation wasn't that strong, tbh and not opposed to switching back.

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

FWIW this was definitely working during Drupalcon Barcelona in late September so our troubleshooting can be in the mindset of "what broke this" vs. "we need to build this". Typically we'd have had tests that would have caught this, but the challenges of getting this set up in JS e2e tests made it a lower priority. Perhaps a PHP FunctionalJavascript test should accompany this fix as those tests are already equipped to handle file uplaods.

Seems fine to switch the response to HTML. I know error handling was one of the concerns mentioned in Slack, but JS error checking/reporting is still captured and displayed properly in the try/catch and the preview itself has Drupal message regions which makes it extra capable at error reporting.

I can't codeowner-signoff because I provided the initial commit with the test, but this solution is AOK - it gets around having to use the libraries info alter hook that is sensitive to the first page visit. What's there is sorta workaroundy, but not moreso than what was removed, and it stops this bug from happening.

balintbrews → credited bnjmnm → .

wim leers → credited bnjmnm → .

Full page without toolbar etc. The XB Content area slot works like it should. IDK if we should do more within this issue scope or just get this in so it's easier to try out different page editing approaches.

bnjmnm → created an issue.

Feedback addressed/responded

Between this issue and 📌 Confirm Semi-coupled form elements can work with State API visibility Active , if we want to continue using Radix we need to have a way to customize the attributes sent to the input elements within the component. I filed an issue with primitives about doing this with Checkbox, and if that is welcomed I'll do the same in Radix themes for the TextField component

The MR has some tests fail - not sure if it's truly broken or conflicts with aspects of the test setup that would not occur in a real Drupal install.

bnjmnm → created an issue.

Re #6 Yep that's what it looks like. cache_discovery - cid library_info:olivero is the culprit. If this cache item is first generated when the XB UI loads, the $route_name === 'experience_builder.experience_builder' condition is true and the library info cache will omit the extra assets. I suspect when that scenario happens then Olivero risks having some assets missing, but I haven't confirmed this.

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

I updated the steps to reproduce and added a failing e2e test that admittedly does not follow the exact steps as summarized, but still runs into the problem. In the test, it fails due to an error vs a non-passing assertion because the pile of Olivero assets include navigation JS that assumes certain elements are present and it errors when those elements aren't found.

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

Needs a @traviscarden signoff because of the change to openapi.yml

When I was working on ✨ Add support for the inline variation of form elements Needs work over four years ago, I created this ussue as this was one of several things that might improve the feasibility of that issue landing. TBH I think it's very unlikely that issue will ever land and getting something like this in will not increase its changes. The changes are too far reaching and disruptive - even if core worked solidly there would be countless problems with the contrib.

I'm just going to close this and someone can reopen if there's a really compelling reason beyond nudging forward a Claro issue that is unlikely to ever be completed.

Discussed this with @longwave, and one big next step here besides general review is to create tests that surface the difference cases where there isn't a 1:1 map between field values, prop values, and widget values. @longwave offered to start on that.

Can we make those separate issues that are referenced here? Aside from this being a large review already, there are major changes to the .module file that require quite a bit of manual steps to reroll.

In 📌 Create extendibility proof of concept that also serves as documentation-by-example Active I've added a test module that provides a barebones implementation of the Semi Coupled theme engine . I think we're running into a usefulness plateau with attempting to explain it all in a single document, and routing some of that documentation to the test module might prove more helpful.

I also added 📌 Create readme for hyperscriptify library Active to create a Readme for Hyperscriptify so that explanation lives within the package that provides the functionality. We can certainly link to it etc but I think having it with the package can help demonstrate that the functionality it provides is not reliant on XB or Drupal.

bnjmnm → created an issue.

Added a test module in the MR and found that the autocomplete inputs (which had been proven to work in the past but never got test coverage) no longer work due to the input now using the Radix TextField component. This component intercepts a few props, including className and applies them to the input's parent <div> instead of the <input/> and Drupal's autocomplete initializes based on the presence of .form-autocomplete on an <input/> element.

bnjmnm → created an issue.

It's probably not all there yet, but this is where i leave off today. The PSD file is available for anyone to edit a copy, too.

Still working on editing the image from #43 it's a chungus Photoshop file.

bnjmnm → created an issue.

This is addressed much more easily with the schemas fully available which is happening in 🐛 Premature prop validation can break the UI Active , so I addressed the reported issue there + included tests to verify it is fixed. Get that one reviewed/codeowner signed and the symptoms reported here are fixed in a less hacky way than what can currently be done off 0.x

The reported bug was addressed in another issue, but the test is worth keeping to ensure this does not regress. Remove the changes to RightClickMenu but keep the test

Feedback addressed

bnjmnm → created an issue.

• I left one small suggestion for rewording things in xb_stark.info.yml

  . Good call, done!

• @bnjmnm, can you please see if you're happy with how I did the form alter in 0332d920? There might be a nicer way to do it.

  It's good.

• On a call last week @jessebaker had the suggestion to use include and refer to the original Twig templates that way in our process_as_regular_twig template overrides. Would we like to do that as part of this issue, or in a follow-up?

  Thanks for the reminder! With that change this is an MR that reduces the total LOC by 139!

Just need a few additional codeowner signoffs.

alexpott → credited bnjmnm → .

Gitlab does not list me as a codeowner that can approve this, which I assume is due to me being responsible for some of the earlier commits on the MR, but all the changes since mine are good by me.

Anything I spotted that I'd typically point out in a review happens to be something that either will be refactored out of existence within a week or two, or related to interactions that don't have formal requirements, so it's better to have this committed so it can help those requirements be discussed.

This is kind of a weird one to approve as there's no great way to test it without literally adding support for non SDC but that is what THIS issue is blocking. Worst case this is a little incomplete but does no harm, so lets let it in to help the other issues move forward.

This still needs src signoff - I'm going to assign to @tedbow who I know is around for the next two days, but anyone in that group could take care of this.

A nice bonus is that the Media Library widget is also rendered with xb_stark

Although XB stark defaults to using Semi Coupled and rendering with React, the MR includes Twig templates that ensure Media Library is still rendered with Twig. This was added to address an issue where core/drupal.dialog.ajax needs the completed HTML earlier than Semi coupled can (currently) provide it. If we really want to render the Media Library widget in React I'm sure this can be done, but it might be worth keeping as is since it's the format the Media Library UI is known to work well with, and there's no need for it to communicate with redux.

This may open options up to more easily address 📌 Media Library dialog styling Active if we want XB specific styles instead of using the admin theme.

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

xjm → credited bnjmnm → .

Good News I think this image helps make sense of some of the more confusing bits
Bad-ish News (more work.. but better code) Seeing it mapped out like this made it clear there are some additional changes I should probably make to make the DX less confusing.

See MR

Can you please confirm that State API is NOT supposed to be working yet for this form? E.g. “Provide a menu link” checkbox.

Can you please confirm that autocomplete inputs are NOT supposed to be working yet for this form? E.g. “Tags” field.

During early evaluation/proof-of-concept stages we confirmed that these functionalities can work, but this was before we had solid tests in place where we could ensure that functionatly continuesto work. It's very possible that something changed in the past several months that changed what is needed to get those working. I don't think this issue scope needs to be concerned with any functionality we have not confirmed to be working with e2e tests. I created 📌 Confirm Semi-coupled form elements can work with autocomplete Active and 📌 Confirm Semi-coupled form elements can work with State API visibility Active

bnjmnm → created an issue.

bnjmnm → created an issue.

The A11y test is catching this contrast error where the bg/fg ratio is 3.26:1 when a smaller control like this needs to be at least 4.5:1

Reinstall takes care of it.

The tests now work - not quite ready to set to NR as we should test scenarios like pasting a component that has been deleted and if possible, pasting between tabs.