- Issue created by @wim leers
- Assigned to gauravvvv
- Issue was unassigned.
- 🇺🇸United States Kristen Pol Santa Cruz, CA, USA
Added link to code in summary.
- 🇺🇸United States Kristen Pol Santa Cruz, CA, USA
Good to know this... we have components that use "name" for props so tracking this:
📌 Track issue that XB doesn't allow props to be called "name" Active
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@fazilitehreem just reported the same bug: 🐛 UI reserves the "name" key in its model, preventing any SDC prop from being named "name" → causes crash for `shoe_icon` and `shoe_tab_panel` SDCs Active , and I had to re-investigate this: #3473525-3: Adding shoe icon and shoe tab panel crashes the site → 😬
Crediting him.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@lauriii, point 6 in the issue summary's needs a clarification/response from you. 🙏
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 [later phase] [PP-2] Prepare for multiple component types: prefix Component config entity IDs with `sdc` Postponed just landed. Component config entity IDs are now present in the client-side data model, instead of SDC plugin IDs. Next up: 📌 Add support for Blocks as Components Active . So one small step in the right direction has already been made! :)
- 🇫🇮Finland lauriii Finland
If product requirement 13. Undo and Redo must also apply to bundle/base fields on the host entity (i.e. Field Widgets in the "regular entity form"), that would also impact the UI data model. Product lead @lauriii should clarify that 😄🙏
The undo/redo should apply consistently to any content and design changes in XB. This means that for consistency, the undo and redo should also apply to changes in the values of base fields and bundle fields. 👍
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Thanks for confirming. That massively complicates things obviously. That means the undo/redo history must track not only the XB component tree (its
tree
structure +props
values), but also the host entity's values — both in server-side representations (to be able to updateprops
values usingDynamicPropSource
s) and client-side representations (to be able to update the corresponding bundle/base field widgets — presumably without triggering AJAX updates to ensure instantaneous reactivity of the UI when undoing/redoing).That's quite a big challenge. Because field widgets today do not support that kind of undo/redo either… 🙈
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@tedbow, @effulgentsia, @longwave, @bnjmnm, @jessebaker, @traviscarden and I just met and discussed this issue.
Notes: https://docs.google.com/document/d/1sMpbWxnOZM-yq4IbrabUBh7-RGYj_2-aYkBR...
We have a rough outline for what we think could/should work.
Next steps:
- first land 🐛 Experiments in rendering Twig as React Active
- then provide concrete examples for each combination in the table that that issue is adding to the docs
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@jessebaker started Assigned to: jessebaker ✨ Get component name from components list not from the component's model Active , which addresses the first point in the issue summary 👍
It's been RTBC since Thursday, but needs final sign-off from @jessebaker. He's back from PTO tomorrow. Postponing on that.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
✨ Get component name from components list not from the component's model Active landed.
I think the most important next step is 📌 Define `props` in the context of Block components Active . I'll try to push this issue forward tomorrow, but ideally it would not be me 😇
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
We'll also probably need a version number or hash to prevent clobbering of other user's edits in multi-user env.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
When you do pick this up next week: see 📌 Add support for global regions Active , where @longwave, @lauriii and @jessebaker articulated already one significant change to support
PageTemplate
: we'll get rid of the special "root" in the client-side data model, in favor of multiple named component trees, with thecontent
region's component tree representing the component tree of the currently edited content entity's XB component tree, if the XB UI is currently looking at a route pointing to a content entity. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
* locked regions so that content-type templates don't impact page templates
How could they ever? Content type templates would be independent of
PageTemplate
config entities? 🤔The challenge I see with content type templates is that they're a single component tree (i.e. comparable to a single region's component tree in a
PageTemplate
), but with the need to have arbitrary subtrees locked. It sounds like you're thinking of naming the editable subtrees? 🤔 But that depends on what the product/UX decisions would be for 🌱 [META] 7. Content type templates — aka "default layouts" — clarify the tree+props data model Active . - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I added
locked regions so that content-type templates don't impact page templates
based on comms in 2 in https://www.drupal.org/project/experience_builder/issues/3489899#mr429-n... 📌 Add support for global regions Active but it sounds like I've represented that wrong
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
my current thinking on how to solve this involves the following:
- stop calling postPreview from a useEffect hook in sendPreviewRequest
- put the previewHtml in the redux store as its own prop, select that in Preview and pass that along as the iframe src
- add a getPreviewHtml to the preview API and add a controller for that, use that for the initial load
- change formStateToStore to instead POST to the preview endpoint, send the current model AND the component ID and the new prop values for that component
- in the preview endpoint do the widget massaging stuff
- return the built previewHTML like preview currently does (top level html key) but also return editedComponentId and newValues (After having run Drupal widget massage etc)
- client side unwrap the response and set the previewHTML in the store using onQueryStarted and updateQueryDate in the POST preview mutation https://redux-toolkit.js.org/rtk-query/api/created-api/api-slice-utils#u.... Also use onQueryStarted in this mutation to dispatch the model change
I think this gets us:
- no additional http request because we're already doing a round trip for the preview when we change values
- drupal/server side is the authority on how to take the form values and manipulate them
- I think we can simplify a fair bit of the logic in things like getPropValues because all that is done Drupal side - in fact I'm pretty sure we can delete that with this in place
I'll start building this out tomorrow
- 🇺🇸United States bnjmnm Ann Arbor, MI
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 like
onQueryStarted
+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
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
-
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.
Great idea, we can also return any invalid entries, but it would require moving the form state context into the global redux slice. I don't think that's an issue and is probably desirable in the long run
-
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.
Yes I think this will likely end up being wiring on the backend to the source plugins with any luck
-
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
I'm not afraid of being wrong in public - I do it all the time. I think its important for newcomers to see experienced campaigners like us get it wrong. It helps with imposter syndrome. To answer your question I _think_ we can avoid an extra request to the backend when we update the model by making use of this feature. We will need to keep the postPreview call in useEffect for other operations that modify the tree but don't need to post to Drupal (new component insert, re-ordering, duplication, delete). This means when the proposed POST to do validation/massaging returns a new model and a new preview, we don't want that update to the model to trigger another postPreview call. I think there' should be a combination of those APIs we can call to prevent that needing an extra round trip if we already have the data. But I could also be wrong, once I get into it I'll know.
One thing this does do is move us further from real time updates because now we're waiting for a round-trip to Drupal before we update the model but really, we weren't updating the preview without the round trip anyway. I think to get to that we'd need to implement the transformers mentioned in the google doc and try to convert as many of the massageFormValues in the BE to transformers. We could retain the round trip when we don't have info on transformers (e.g. for widget classes we don't know about in contrib). But all of that is a long way off because we don't have real time preview updates yet anyway
-
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Pushed a WIP to branch
3467954-server-side-model-massage
will continue tomorrow - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Made some good progress here, server side model validation and massaging is happening in a single request that also updates the preview
Work to date https://git.drupalcode.org/issue/experience_builder-3467954/-/compare/0....
Working on removing the media workarounds at present
- Merge request !448Draft: Resolve #3467954 "Server side model massage" → (Closed) created by larowlan
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
📌 [PP-1] Set form state values in bulk rather than in inputBehaviours Postponed and 📌 Move form state into the global store Active are blocking this
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Added child 📌 Maintain a per-component set of prop expressions/sources Active which is a discrete next step with manageable scope
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Added 📌 Split model values into resolved and raw Active as another discrete step
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
📌 Move form state into the global store Active is in
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Add support for global regions Active is in!
- 🇺🇸United States effulgentsia
Per #44, is this still blocked on 📌 [PP-1] Set form state values in bulk rather than in inputBehaviours Postponed ?
- 🇺🇸United States effulgentsia
Has the work in this issue turned up specific cases of 📌 Discover cases where is no 1:1 map between field, prop and widget values Active that could be documented on that issue? Also, I'm hoping some of the work here helps elucidate how to then implement those cases as they're discovered.
- 🇺🇸United States effulgentsia
#3491978-7: Implement saving block settings forms → raises an interesting question about block settings forms having some characteristics of props forms and some characteristics of page data forms, and I'm wondering if the work done in this issue helps make that clean.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Updated issue summary with the 5 issues I think we need to resolve here to mark this as done.
I've put them in order I think they should be tackled
- 📌 Remove dynamic prop sources from test data Active this can be actioned
- 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active this can be actioned and will help 📌 Implement saving block settings forms Active
- 📌 Maintain a per-component set of prop expressions/sources Active this has a WIP branch that is blocked on ^ and is passing PHPUnit tests
- 📌 Split model values into resolved and raw Active this is blocked on ^ but there are @todos in that branch for where the works is required
- 📌 Support server side massage and validation of component prop form values Active there is a WIP branch in this issue for this, I will rebase and move it over there. We probably should wait until 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active is done first so that we keep block settings moving
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Closing MR 448 as that has moved to https://git.drupalcode.org/project/experience_builder/-/merge_requests/536 and 📌 Support server side massage and validation of component prop form values Active
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Remove dynamic prop sources from test data Active is in, as is 📌 Implement saving block settings forms Active .
AFAICT 📌 Maintain a per-component set of prop expressions/sources Active is the next big one, but 📌 Move SDC specific validation in ValidComponentTreeConstraintValidator::validate into the SDC source plugin Active and 📌 Remove references to 'props' outside of SDC - use 'inputs' instead Active would also help get us to a better place.
- Assigned to larowlan
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active is in!
Let's get this done 🤝
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
IMHO 📌 Allow source components to control how hydrated values are mapped into the clientside model Active should happen before 📌 Maintain a per-component set of prop expressions/sources Active . See https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... for why.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Maintain a per-component set of prop expressions/sources Active just landed.
It feels like 📌 [PP-2] Remove InputBehaviorsBlockSettingsForm and consolidate with input components form Active should be considered part of this meta by the way.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Added (the trivial) 📌 [PP-1] Rename PropSourceComponent.field_data to propSources Postponed .
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Agree re 📌 [PP-2] Remove InputBehaviorsBlockSettingsForm and consolidate with input components form Active - added it
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 [PP-2] Remove InputBehaviorsBlockSettingsForm and consolidate with input components form Active landed yesterday!
📌 Support server side massage and validation of component prop form values Active is in review, with currently one known FE and one known BE regression. I'm confident we'll be able to fix those.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Added child meta 📌 Discover cases where is no 1:1 map between field, prop and widget values Active
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Split model values into resolved and raw Active is in!
Unless you oppose, I'd like to mark this meta as fixed — 📌 [PP-1] Rename PropSourceComponent.field_data to propSources Postponed is a clean-up and 📌 Discover cases where is no 1:1 map between field, prop and widget values Active is a meta of its own, with a different focus.