🇳🇱Netherlands @balintbrews

@bnjmnm is kind enough to take over for the rest of this week as I'll be mostly on holiday until early January.

Here is what's left:

1. Reroll to incorporate changes from 📌 Move form state into the global store Active ;
2. Sending page data to backend;
3. Test coverage;
4. Code clean-up.

Follow-up issues opened:

• 📌 [PP-1] Handle media image fields on page data form Postponed ;
• 📌 [PP-1] Handle fields with multiple form control elements on the page data form Postponed .

balintbrews → created an issue.

balintbrews → created an issue.

wim leers → credited balintbrews → .

I ruled out my theory for the reason behind the test failures. Adding cy.wait() in those tests doesn't make them fail — it should surface the problem if it was a race condition with the component re-renders.

Then I remembered something @jessebaker said on Slack: maybe it's a dev vs. production build issue. That's it! 💫

I was able to confirm that testing the app after npm run build (and reverting my fix) doesn't present the issue! This is because there are no subsequent re-renders occurring for the InputBehaviors component in the production build. They happen in development mode thanks to our use of <StrictMode>, which helps us catch bugs by double rendering in development. That's exactly what happened here. The fix is still valid and necessary.

I don't see the need for updating our tests: the production build was not broken. Lower-level component tests could test this, but I feel like that would be out of scope for this issue.

The problem is now fixed. I re-introduced maintaining a local state for the input values. This is necessary because we remove the value attribute from the input element in order to avoid getting warnings from React that a controlled component doesn't have event handlers. (See the relevant code in 0.x.) With this attribute removed, reading the value of text inputs on subsequent re-renders — of which we do have a couple — is not possible. Unless we have it saved in a local state.

I don't fully understand how this was not caught by the tests, but I do have a theory. The only thing I can think of is that the value attribute is there on the initial render, which I was able to confirm via debugging, but I wasn't able to confirm in a way that would actually paint it on the UI. The test code might have been able to get a hold of that value attribute before the component got re-rendered, and deemed the assertion true.

Draft code is up showing the idea:

1. InputBehaviors wraps the input in one of InputBehaviorsComponentPropsForm or InputBehaviorsEntityForm. (We can also add a fallback later.)
2. These wrappers both wrap the input in InputBehaviorsCommon which essentially does what InputBehaviors did before, but using the following callbacks it receives as props:
   • commitFormState;
   • parseNewValue;
   • validateNewValue — with an optional setInputMessages argument.

This new approach already works and does everything it did before for the component props form.

Undo/redo isn't working yet for the page data form. I need a way to initially get the entire entity data, so I can put it in the Redux store all at once instead of saving each input one by one, which would pollute the history for undo/redo. I chatted about this with @larowlan, I'll return that data in the response of \Drupal\experience_builder\Controller\ApiLayoutController for now, and save it to the appropriate slice in onquerystarted.

@jessebaker wisely suggested that I consider extracting a piece of this work into a separate issue, and it made a lot of sense to do so for #4.4 ✨ Save page data form values in application state with support for undo/redo Active : 📌 Split form components into `Drupal`-prefixed behavioral wrappers and presentational components Needs work .

It's nice to wrap up that piece, and it will make the review of this issue much easier. 😌

Created 📌 Create `FormElement` and `FormElementDescription` presentational components Active as a follow-up issue for an improvement.

balintbrews → created an issue.

balintbrews → created an issue.

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

Fantastic work! 👏🏻

FYI, as I was refactoring the Accordion component in ✨ Save page data form values in application state with support for undo/redo Active , I lifted @bnjmnm's awesome solution from this issue to handle <details> outside of vertical tabs: c8d73a03. We will have separate DrupalVerticalTabs and DrupalDetails components which will include AccordionRoot+AccordionDetails and Details, respectively, with a single CSS file that also accounts for animation for both.

I'm happy to help with merge conflicts if and when they need to be resolved. 😊

#19 📌 Preview entire page not just content area Active :

That quote from @lauriii is what I also expressed in #13 📌 Preview entire page not just content area Active .

My interpretation is that we don't intend to display errors as part of the generated preview, but that doesn't mean we don't intend to display errors in the XB app somewhere when generating the preview goes wrong. Likely in a toast, as mentioned in 📌 Improve server-side error handling Active .

The question is how granular we would like to be with these errors. If not so much, i.e. we're happy with two different kind of generic errors, then #17 📌 Preview entire page not just content area Active works! I guess many other scenarios can be expressed with HTTP codes, so this can even scale to more errors.

I will say, I don't get why we are putting so much emphasis on this when have a system worked out for JSON responses with error handling, already implemented, used by many other endpoints. I'm probably failing to understand some benefit of this potential change. 😊

Good callout! It's out of scope for this issue, but I'm definitely keeping an eye on 📌 Save metadata(page data) field with the entity revision Active to make saving easier — maybe quite soon.

I would like to propose that we reconsider switching to HTML as the response format and stick to JSON.

As @lauriii mentioned in #11 📌 Preview entire page not just content area Active , error messages shouldn't appear as part of the preview. This is in line with our aspiration to keep the rendered preview as intact as we can, so it truly represents what end users will see.

In order to handle errors outside of the preview, we do need them in a JSON response. @longwave's suggestion to set the responseHandler option to content-type for the endpoint generated by RTK Query would solve this elegantly: the response would be parsed according to the Content-Type header from the response, so it's possible to mix HTML and JSON responses.

While that is certainly a solution, I'm concerned that RTK Query's abstraction hides the fact that we would be creating an unusually complex client requirement. Our implementation happens to solve it easily through an abstraction, and while I don't pretend to foresee that there will be other clients consuming this API endpoint, it's probably a good idea not to rule that out, even just for the sake of designing a better API.

I think it's overall a better API design to keep a consistent response format.

What is the problem that we're trying to solve with switching to HTML response? Debugging was mentioned on Slack. What would be the exact scenario?

I discussed next steps with @bnjmnm on a call last week. We agreed on the followings:

1. We will make use of FormStateContext implemented by @/components/form/components/Form to provide the information of what form is being rendered: currently this will mean the component props form or page data form.
2. InputBehaviors can make use of this information and return different higher-order components (or the same with different props) to adjust the behaviors according to the form's needs.
3. We will not change how the Twig to JSX mapping is done, so reverting my previous commits, twig-to-jsx-component-map.js will stay unchanged.
4. All mapped components will be split into two (or more) components, where a component with its name prefixed with Drupal will render another component (or components) which are purely presentational. The Drupal-prefixed component will wrap the component(s) it renders in InputBehaviors. This is what I already started in 1631cd12. The separation will help with our Storybook implementation, implementing designs in isolation etc.

Here is a high-level overview of what InputBehavior does today. Validation and store update are the two areas where we need to allow different logic for each form. They're currently implemented for component props.

I think @bnjmnm would be the best person to review the solution.

🚀 This will make ✨ Save page data form values in application state with support for undo/redo Active much cleaner to implement, because we can simply display all (base) fields from this new entity type instead of dealing with nodes before we figure out 📌 [later phase] [RESEARCH] How to identify all meta fields for an arbitrary content entity? Active .

I just learned about @brianperry's module, Islands → . We could explore how to leverage it for our hydration logic with Astro.

balintbrews → created an issue.

#17:

It would probably be good for the Astro project to be a separate project (meaning, have its own package.json) from the main XB project. This separate project could be either a directory that's a sibling of the ui directory, or a child. For example, astro or ui/astro (or we might want to come up with some other name for it than just calling it astro). In which case, we'd want the build script in ui/package.json to do whatever it does plus then kick off an npm run build command within the astro directory. I'm guessing there's idiomatic conventions for how to structure/implement this type of setup, but I don't know what that is.

Tools we can evaluate that comes to my mind are Yarn workspaces, Lerna, or Nx (also uses Lerna, probably an overkill for us, but it's an awesome tool).

That question on StackOverflow is quite old, and it also shows that the Next.js team was not super concerned about the warning either. 😊

typescript-eslint has support for the latest stable version of TypeScript, 5.6, since its v8.10.0. If we're changing version numbers to get rid of this warning, I would rather get us upgraded to newer versions than going backwards.

@shyam_bhatt, are you up for giving this a try?

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

While the warning is harmless in our case, I agree that it may be confusing and distracting.

Let's not downgrade the TypeScript version. Instead, can we please see if we can update to a newer version of typescript-eslint? We could also set warnOnUnsupportedTypeScriptVersion to false in our ESLint config, but let's try to avoid that.

Here is an outline of our approach for handling Tailwind-generated CSS — developed based on several meetings with @effulgentsia, and another meeting with @effulgentsia, @hooroomoo, @tedbow, @longwave, and @f.mazeikis.

Experience Builder's Tailwind CSS support for JavaScript components

JS components in XB have two target groups:

1. Marketing teams with sufficient level of design and frontend coding skills to author (and/or use LLMs to generate) basic components, with no or minimal interactivity, mostly consisting of markup.
2. Developers, who can create advanced, potentially highly interactive components.

80-90% of the JS components will be authored by marketing teams. Therefore we will consider XB as the primary source of truth for JS components. This mostly means that the Tailwind CSS 4 config will be maintained by the Experience Builder module, rather then e.g. residing in an external code repository.

Basic components

Marketers can write Preact components using an in-browser editor. They can also maintain their Tailwind CSS 4 config using Experience Builder. Tailwind CSS 4 is still in alpha, but there are already examples of it being used in production. One of the great new features is CSS-first configuration.

We are already able to build CSS using Tailwind CSS 4 in the browser via a new package that has been authored and published on npm: tailwindcss-in-browser. Using this we will produce the following CSS files using the components' markup and the Tailwind CSS 4 config maintained in XB:

1. Tailwind Preflight & Tailwind theme CSS;
2. Individual CSS files with Tailwind utility classes for each component.

Advanced components

Developers can develop components outside of Experience Builder using any workflow that fits them, e.g. keeping their code in a code repository. XB will provide a CLI tool to support the followings:

• Preview components by building CSS using the Tailwind CSS 4 config, retrieved from XB on-the-fly;
• Deploy components to XB with their built CSS — an individual CSS file for each.

CSS aggregation

Every JS component, basic or advanced, will end up with their own CSS file. While this will ensure great portability and reduces complexity for the initial implementation, it also results in a great deal of duplication in the CSS code. It was agreed upon that this is acceptable for 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active , and can potentially be addressed later in Drupal core, implementing de-duplication as part of the CSS aggregation process.

balintbrews → created an issue.

This is looking great! I requested two code changes. Besides that, as Wim already pointed out correctly in #13, we still need to provide some test coverage for the logging that was added in this MR.

Ideally we would do that by mocking our `log` module in our existing `ui/tests/unit/error-boundary.cy.jsx` test, but mocking modules in Cypress component tests is problematic, and given we have the ongoing discussion of 📌 Consider dropping Nightwatch in favor of Functional Javascript tests Active , which may result in changing the testing framework of XB in the future, I don't think we should try to solve it. We can still verify the logging in an end-to-end test, there is already one in the repository where we intercept a request and mock an error form the server (`Handles and resets errors` in `ui/tests/e2e/xb-general.cy.js`). That test can be extended to intercept and verify the request sent to `/xb/api/log-error` when the error boundary is rendered.

This is still very much in progress, but here is a sneak peek into where we are currently.

I fixed the problems I mentioned in #15, then reviewed all code changes, and approved the MR.

I have three questions/asks.

1. I left one small suggestion for rewording things in xb_stark.info.yml.
2. @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.
3. 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?

Changes were introduced upstream by 📌 [PP-1] Make "page data" tab in right sidebar work Postponed that needed to be adjusted to this MR. Needs work:

1. The page data form is broken after the merge for some reason;
2. A form alter needs to be added back to hide the submit buttons on the page data form;
3. \Drupal\Tests\experience_builder\Functional\EntityFormControllerTest::assertFormResponse needs to be updated.

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

I went through all changes and approved the MR. Thanks for the hard work on this, everyone!

We're blocked on a code owner approval for the changes in gitlab-ci.yml. I naïvely thought I could temporarily add myself as a code owner, and give the approval, but it didn't work. (Makes sense that it didn't.) I assume that would need to be done upstream. I opened 📌 Update "DX: CI" section of CODEOWNERS Active .

balintbrews → created an issue.

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

Here is where we ended up with this issue:

I need to slightly adjust a test now that the other issue landed.

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

This is now ready for review. I think the best person to do the first round would be @bnjmnm. 😊

The MR description outlines what has been done and what is out of scope for this particular issue.

That's helpful, thanks, @bnjmnm!

#17: Great, so Stark is already the negotiated theme for those routes. I started to investigate how I'm still seeing Olivero's CSS added in the XB app, then after a clear cache etc. it's gone. Now the task is to figure out how to reproduce it being added. 🙂

@bnjmnm, I would appreciate your input on a few questions I have.

1. Both \Drupal\experience_builder\Controller\EntityFormController::form and \Drupal\experience_builder\Form\ComponentPropsForm::buildForm render their responses using the current Drupal theme. This means that the theme assets are added, including its stylesheets, which may contain CSS rules with high specificity (e.g. selectors targeting HTML tags directly). This became very visible and problematic as I started mapping checkboxes to a React component: I found myself writing CSS reset code for those. Have you thought about this situation? Could we switch the theme to e.g. Stark in those controllers? Or maybe you already have a much better idea? Knowing the general direction here would help me decide what is reasonable to aim for with my styling code in the scope of the current issue.
2. Can you please confirm that State API is NOT supposed to be working yet for this form? E.g. “Provide a menu link” checkbox.
3. Can you please confirm that autocomplete inputs are NOT supposed to be working yet for this form? E.g. “Tags” field.

The ability to recover from an issue is what I would consider the main factor when it comes to deciding severity. The error boundaries can catch all kinds of problems, and we can't really predict with full certainty when the UI will be able to recover from those. Therefore I recommend that we go with the level error by default.

Some errors are thrown, or could be thrown, explicitly by our own code, and while we could implement our own class extending Error and add the severity on a case-by-base basis, I don't see a lot of value in that. We wouldn't be able to make it very granular anyway.

I would make an exception with the RouteErrorBoundary and RouteAsyncErrorBoundary components. When those components are rendered, it's fully certain that there is no recovery other than reloading the page. I would set the critical level for those.

I'm curious what others think.

I missed documenting it in this issue, but I discussed the resizing of the contextual panel with @lauriii last week, and this is an interaction that needs some design from @bostonjillian and @Renee Lund. Instead of simply allowing to resize the panel, we may want to trigger size changes based on actions that happen inside the panel.

I would like to keep this assigned to myself to work out details while I'm focusing on 📌 [PP-1] Make "page data" tab in right sidebar work Postponed and its follow-up issues.

Nice fix, @soaratul! 🎉 I forgot to do this when I introduced the Panel component in ✨ Implement new design for top bar and sidebars Active : https://www.radix-ui.com/primitives/docs/guides/composition#your-compone....

We'll need one more approval because of a CODEOWNERS rule.

Adding the latest screen capture. 📽️

I ended up adding back the Scroll Area component from Radix Themes. With the significantly cleaned up layout, it works as intended. The close icon is now removed after discussing it with @lauriii — we'll open a new issue to refine the interactions with the sidebars.

I reviewed the docs, improved punctuation and language, added a couple of links, and extended the hyperscriptify example code snippet. I think the content has the right level of details and is easy to follow. I hope that's helpful! 🙂

Reassigning for reviewing backend changes.

Happy to review this.

We need 🐛 Unable to scroll component props form Needs review to make the sidebar usable. Not marking this issue as postponed on it just yet, there's plenty to do even without 🐛 Unable to scroll component props form Needs review . I'll be working on these two in parallel.

I'm picking this up again, we need it for 📌 [PP-1] Make "page data" tab in right sidebar work Postponed .

#11: Works like a charm! 😊 What I asked for ++.

Most of the form is now rendered as React components. A lot more to do, but we're off to a good start!

Even though the form is marked as #is_xb in the initial code by @bnjmnm, no element inside is getting that attribute, so we're not getting them mapped to the JSX components. I managed to find out why.

Marking all children of an element if it has #is_xb is supposed to happen in \Drupal\experience_builder\EventSubscriber\XbMainContentViewSubscriber. This event subscriber is subscribed to the KernelEvents::VIEW event. That event is not getting triggered when a controller returns a JsonResponse. Which is what \Drupal\experience_builder\Controller\EntityFormController (added in 📌 HTTP API: new /xb/api/entity-form/{entity_type}/{entity}/{entity_form_mode} route to load form for editing entity fields (meta + non-meta) Fixed ) does.

Would it be okay to update \Drupal\experience_builder\Controller\EntityFormController to return a render array instead? That would match the behavior of \Drupal\experience_builder\Form\ComponentPropsForm::buildForm. This simple change would immediately solve the mapping problem and unblock this issue. (I verified this with a quick test.)

(My gut feel is that long term we may want a JsonResponse for both controllers so it's more intuitive what's happening even just by glancing at the HTTP response. Not sure if you agree, but either way, that could be a new issue if desired? 😇)

😇

Let's rename the directory to tests, please: ui/cypress → ui/tests. That is more conventional, and it's how people would look for tests. I think the current structure under that folder is fine, I don't see the need to have a subdirectory named cypress.

Also, when I tested the changes locally, I got many ESLint errors and required formatting changes when running npm run lint:eslint and npm run format. Let's make sure those are all gone before we merge the MR, otherwise we will break all MRs that merge in upstream code.

I really like this solution! 👏🏻

The MR is a rough draft to implement the new design. It requires more work to use more CSS variables, match the design etc.

balintbrews → created an issue.

<form method="dialog"> is beautiful! 👏🏻

I was just thinking that a simple end-to-end test would be reassuring to make sure we don't accidentally remove it when generating the form, then face this awkward bug again. 😊

After re-running the pipeline, things are green now! 🟢

Here is what I suggest we do next:

1. Change the implementation to use the HTMLFormElement: submit event. As I mentioned before in #3, I think it's more semantic to intervene directly in the event that causes what we would like to prevent, instead of intercepting the event that leads to said event. We would like to ensure that if and when the form gets submitted, none of the default actions happens. How the form gets submitted, e.g.. as a result of the Enter keypress, is an implementation detail.
2. Even though we already fixed the reported bug, I think we should remove the action property from the generated form markup returned in \Drupal\experience_builder\Form\ComponentPropsForm::buildForm, so the form markup doesn't express something that is not expected.
3. Tests would be nice to validate this.

Assigning to Wim Leers to get some feedback on the above.

🐛 The XB annotations and labels should not change size when zooming Needs work landed 🚢 — and with that, our backlog is cleared. 😊🏁

Tests were recently written to cover enum fields as part of 📌 Redux Sync all single-value types in the SDC test all props form Fixed . With that covering the basics, let's postpone this issue until we have specific UX created for component variants.

While I was convinced before that this should be handled where the form is generated — the backend —, after more thinking, and mostly based on your findings, I have to realize that it may not be enough.

I think on the frontend we should consider leveraging the HTMLFormElement: submit event, and Event: preventDefault() method to prevent the browser doing the default action upon submitting the form. It feels more semantic to me than handling a specific keypress, but I'm curious what others think.

Great work on the tests and everything, @utkarsh_33, thank you! I left you some comments on the test code. I also would love us to explore if there's a better way to address the button width issue.

I have a working MR (!325) for scaling the not-to-be-scaled XB annotations by the inverse of current scale. I think this approach will work!

Here is what's left:

1. More manual testing that the solution indeed works.
2. The "Add section" / "Add component" button starts to look broken at large zoom levels for some reason.
3. Adjusting an existing test to assert that the outer <div> outputted in Outline.tsx has the inline style attribute that cancels out the scaling when zoom is used. This can go into the test case named "Can zoom the canvas with the Zoom Controls".
4. Writing test that ensures that the dimensions of the outline (border shown around selected/hovered component in the preview) are adjusted when zoom is used. This can be as simple as looking at what getBoundingClientRect() returns for the outline's DOM element when zoom level is at 100%, and checking how that changes at different zoom levels.

@utkarsh_33, anything you can tackle from the list above as you start your day on Monday would be super helpful. Let's make this happen before DrupalCon! 💪🏻

Brilliant ideas! While using portals would be a solid approach, I was thinking we could try the second idea first — scaling the not-to-be-scaled components by the inverse amount —, given that if we can make it work, it's probably easier, and we may do a rewrite anyway if we decide to go with 🌱 [Proposal] A different approach to the iFrame preview interactions Active .

🐛 Keyboard commands to zoom in and out should only impact the preview canvas Active landed 🚢

Given the complexities, I think we landed this issue at a very good place, with great analysis on where to go from here, and a clean solution for an incremental improvement. We have two follow-up issues:

1. 🐛 Pinch gesture zooming sometimes invokes OS zoom behavior Active
2. 🌱 Consider a11y impact and/or competitor analysis for preventing browser zoom Active