Overview

Extracted from #3467499: Use a custom serialization format instead of theming to render the edit form to the intermediary representation that XB's React code consumes based on comments 11–17 there.

Route experience_builder.component_props_form uses a form \Drupal\experience_builder\Form\ComponentPropsForm
This form mixes form API and React components via the semi-decoupled theme engine.
This theme engine:

  • Takes over the default twig theme engine
  • Has hardcoded special treatment of theme suggestions that end in _xbxb
  • Uses json in twig to map Form API properties to JSX data-types

Proposed resolution

User interface changes

None.

CommentFileSizeAuthor
#12 xb-stark-renders-ml.png847.46 KBbnjmnm
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

wim leers created an issue. See original summary.

johnwebdev’s picture

Issue summary: View changes
wim leers’s picture

Thanks, @johnwebdev, appreciate that 😊

deepakkm made their first commit to this issue’s fork.

deepakkm’s picture

Status: Active » Needs work
wim leers’s picture

Component: Page builder » Semi-Coupled theme engine
Assigned: deepakkm » Unassigned

@deepakkm is no longer working on XB.

wim leers’s picture

Assigned: Unassigned » bnjmnm
Priority: Normal » Critical
Status: Needs work » Needs review

… and AFAICT this will allow us to delete the very existence of the #is_xb render array property — because this MR will make it possible to simply always use the xb_stark theme for certain routes.

And with #is_xb obsolete, we'll be able to delete ALL infrastructure around it (that sets it when appropriate):

  • \Drupal\experience_builder\RenderArrayXB::markXB()
  • \Drupal\experience_builder\RenderArrayXB::findAndMarkXB()
  • \Drupal\experience_builder\EventSubscriber\XbMainContentViewSubscriber
  • experience_builder_preprocess_form_element()
  • experience_builder_form_component_props_form_alter()

… and experience_builder_theme_suggestions_alter() will be able to remove its first if-statement.

Am I missing something, or can you confirm this, @bnjmnm? 😄

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

wim leers’s picture

🤩

wim leers’s picture

Status: Needs review » Needs work

This MR looks great! (And once the commented out lines in the .module file are deleted, it'll pass phpcs too).

Just one small remark, really.

And … we of course need to get the tests passing! :D

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
Status: Needs work » Needs review
StatusFileSize
new847.46 KB

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 #3471978: Make the Media Library dialog look like the admin theme without materially affecting anything outside of the dialog (possibly use an iframe for CSS isolation) if we want XB specific styles instead of using the admin theme.

bnjmnm’s picture

Assigned: Unassigned » balintbrews

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

balintbrews’s picture

Status: Needs review » Needs work

Changes were introduced upstream by #3469235: Make "page data" tab in right sidebar work 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’s picture

Assigned: balintbrews » bnjmnm
Status: Needs work » Reviewed & tested by the community

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?
bnjmnm’s picture

Assigned: bnjmnm » Unassigned
  • 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.

wim leers’s picture

Assigned: Unassigned » bnjmnm
Status: Reviewed & tested by the community » Needs work

#12: Nice! 👏

#17:

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

😮


This looks great, but there's some loose ends on the docs side. I'd like to see those resolved before this lands, because otherwise the burden of fully documenting how this works would shift to #3446434: Document "Semi-Coupled Theme Engine" and "Redux-integrated field widgets" components.

bnjmnm’s picture

Status: Needs work » Reviewed & tested by the community

Feedback addressed

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
wim leers’s picture

Let's ship this! 🚢

wim leers’s picture

Status: Reviewed & tested by the community » Fixed

End-to-end tests failed randomly, differently: run 1 vs run 2.

⇒ Merging!

  • wim leers committed c5b5d93d on 0.x authored by deepakkm
    Issue #3478287 by bnjmnm, deepakkm, balintbrews, wim leers: Introduce...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

wim leers’s picture

While blogging about this, retro-actively crediting @larowlan for raising #3467499 and @effulgentsia for the fundamental idea implemented here, which he described in #3467499-13: Use a custom serialization format instead of theming to render the edit form to the intermediary representation that XB's React code consumes! 👏