Closed (fixed)
Project:
Experience Builder
Version:
0.x-dev
Component:
Page builder
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
20 Aug 2024 at 15:43 UTC
Updated:
14 Nov 2024 at 15:59 UTC
Jump to comment: Most recent, Most recent file
from the
Comments
Comment #2
wim leers#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
are at least partially working. (See #3454094: Milestone 0.1.0: Experience Builder Demo.)
Comment #5
bnjmnmI 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.
Comment #6
balintbrewsComment #9
balintbrewsEven though the form is marked as
#is_xbin 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_xbis supposed to happen in\Drupal\experience_builder\EventSubscriber\XbMainContentViewSubscriber. This event subscriber is subscribed to theKernelEvents::VIEWevent. That event is not getting triggered when a controller returns aJsonResponse. 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\EntityFormControllerto 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
JsonResponsefor 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? 😇)Comment #10
bnjmnmI'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
JsonResponsealso 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 bothJsonResponseComment #11
wim leers#5: nice start! 🤩🚀
#9: Hm, yes, that was an oversight 🙈 It really should have
just like the
experience_builder.api.component_props_formroute 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! 🙈
Comment #12
balintbrews#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!
Comment #13
balintbrewsWe 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.
Comment #14
wim leers🥳
Comment #15
wim leers#3471108: Unable to scroll component props form is in :)
Comment #16
balintbrews@bnjmnm, I would appreciate your input on a few questions I have.
\Drupal\experience_builder\Controller\EntityFormController::formand\Drupal\experience_builder\Form\ComponentPropsForm::buildFormrender 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.Comment #17
wim leers#16:
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.
Comment #18
balintbrews#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. 🙂
Comment #19
bnjmnmDuring 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.
Comment #20
balintbrewsThat's helpful, thanks, @bnjmnm!
Comment #21
wim leersComment #22
balintbrewsThis 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.
Comment #23
bnjmnmGitlab 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.
Comment #24
effulgentsia commentedI 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.
Comment #25
balintbrewsI need to slightly adjust a test now that the other issue landed.
Comment #27
balintbrewsHere is where we ended up with this issue:
Comment #28
wim leers🤩 @balintbrews GIFs never disappoint! 😁