Overview

Blocked on #3463988: HTTP API: new /xb/api/entity-form/{entity_type}/{entity}/{entity_form_mode} route to load form for editing entity fields (meta + non-meta). That will provide the back-end routes to power from the design.

Proposed resolution

User interface changes

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.

wim leers’s picture

Title: [PP-1] Make "page data" tab in right sidebar work » Make "page data" tab in right sidebar work
Status: Postponed » Active

#3463988: HTTP API: new /xb/api/entity-form/{entity_type}/{entity}/{entity_form_mode} route to load form for editing entity fields (meta + non-meta) is in.

👇

This is now actionable, which would get us to a point where the user stories

  1. As a content creator, I want to edit meta fields directly within the page builder interface. For example, the menu links and URL alias should be editable on the same page as the page builder.
  2. As a content creator, I want to edit fields directly within the page builder interface. For example, the title should be editable on the same page as the page builder. I expect the page title to be displayed in the preview.

are at least partially working. (See #3454094: Milestone 0.1.0: Experience Builder Demo.)

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

bnjmnm’s picture

Version: » 0.x-dev
StatusFileSize
new407.11 KB

I created an MR with a good starting point for someone to pick up - the form is now rendering in the "Page Data" tab, and using React.

There's plenty to do in addition to me getting the form to appear there. There are fields there we don't want, some of the code used to get this working might be repeating logic that could be reused, the current rendering is too wide for the available width, etc etc.

balintbrews’s picture

Assigned: Unassigned » balintbrews

balintbrews’s picture

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 #3463988: HTTP API: new /xb/api/entity-form/{entity_type}/{entity}/{entity_form_mode} route to load form for editing entity fields (meta + non-meta)) 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? 😇)

bnjmnm’s picture

I'm not particularly attached to either return type and don't recall why the props form was not a JSON response. It sounds like it's not a huge effort to switch it to a render array, and switching the other response to JsonResponse also seems OK. For this issue, I'd personally recommend whichever switch requires the least amount of effort, and if it happens to be the Render Array approach we can have a followup to look into making them both JsonResponse

wim leers’s picture

Status: Active » Needs work
Parent issue: » #3450592: [META] Front-end Kanban issue tracker
StatusFileSize
new2.02 KB

#5: nice start! 🤩🚀

#9: Hm, yes, that was an oversight 🙈 It really should have

    # The XB equivalent of requesting an AJAX response using `?_wrapper_format`.
    # @see Drupal.Ajax.AJAX_REQUEST_PARAMETER
    # @see \Drupal\Core\Ajax\AjaxResponse
    # @see \Drupal\experience_builder\Render\MainContent\XBEndpointRenderer
    _wrapper_format: 'xb_endpoint'

just like the experience_builder.api.component_props_form route definition.

Attached is what I think all you should need to do to A) make it not return JSON, B) make it match exactly the thoughtful principles that @bnjmnm and I brought to the ComponentPropsForm. (That means I disagree with @bnjmnm in #10: going either way does not make sense to me, we should follow the pre-existing pattern, so we can reuse all the infrastructure we already added previously.)
Sorry for the confusion! 🙈

balintbrews’s picture

#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!

balintbrews’s picture

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

wim leers’s picture

🥳

wim leers’s picture

balintbrews’s picture

@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.
wim leers’s picture

#16:

  1. render their responses using the current Drupal theme
    Are you sure? 🤔 They should be rendered using stark, see \Drupal\experience_builder\Theme\XBThemeNegotiator::$routes, added in #3469686: Changing default theme can break HTML forms generated by XB API routes: always negotiate the Stark theme.

Assigned to @bnjmnm for the remaining 2 questions.

balintbrews’s picture

#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 cache clear etc. it's gone. Now the task is to figure out how to reproduce it being added. 🙂

bnjmnm’s picture

Assigned: bnjmnm » Unassigned

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 continues to 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 #3481717: Confirm Semi-coupled form elements can work with autocomplete and #3481719: Get Semi-coupled Checkbox form elements to work with the State API to target that functionality.

balintbrews’s picture

Assigned: Unassigned » balintbrews

That's helpful, thanks, @bnjmnm!

balintbrews’s picture

Assigned: balintbrews » bnjmnm
Status: Needs work » Needs review

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.

bnjmnm’s picture

Assigned: bnjmnm » Unassigned

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.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

I approved the remaining thing that needed codeowner approval. I tried to merge this, but it said a pipeline is failing. I suspect due to #3475759: Implement a different approach to the iFrame preview interactions not being merged into this fork yet (or this fork rebased on top of that), but not sure.

balintbrews’s picture

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

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

  • balintbrews committed 01be1a70 on 0.x authored by bnjmnm
    Issue #3469235 by balintbrews, bnjmnm, wim leers: Make "page data" tab...
balintbrews’s picture

Assigned: balintbrews » Unassigned
Status: Needs work » Fixed
StatusFileSize
new8.3 MB

Here is where we ended up with this issue:

Node form displayed under Page data tab, rendered in React

wim leers’s picture

🤩 @balintbrews GIFs never disappoint! 😁

Status: Fixed » Closed (fixed)

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