Closed (fixed)
Project:
Experience Builder
Version:
0.x-dev
Component:
Page builder
Priority:
Major
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
14 Nov 2024 at 13:51 UTC
Updated:
30 Jan 2025 at 13:39 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #4
balintbrewsI discussed next steps with @bnjmnm on a call last week. We agreed on the followings:
FormStateContextimplemented by@/components/form/components/Formto provide the information of what form is being rendered: currently this will mean the component props form or page data form.InputBehaviorscan 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.twig-to-jsx-component-map.jswill stay unchanged.Drupalwill render another component (or components) which are purely presentational. The Drupal-prefixed component will wrap the component(s) it renders inInputBehaviors. 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
InputBehaviordoes 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.Comment #5
larowlanJust flagging that this might also need to update the preview API rtk mutation to also include the form data, and then we'll need corresponding updates to the auto-save controller on the backend.
I did see somewhere that we were talking about setting props on the root to cover the entity form data, so that would bypass any FE changes to the preview API and only require BE changes.
Comment #6
balintbrewsGood callout! It's out of scope for this issue, but I'm definitely keeping an eye on #3488368: Also convert metadata (page data) fields in ClientDataToEntityConverter to make saving easier — maybe quite soon.
Comment #7
balintbrewsComment #8
balintbrews@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: #3491265: Split form components into `Drupal`-prefixed behavioral wrappers and presentational components.
It's nice to wrap up that piece, and it will make the review of this issue much easier. 😌
Comment #9
balintbrewsDraft code is up showing the idea:
InputBehaviorswraps the input in one ofInputBehaviorsComponentPropsFormorInputBehaviorsEntityForm. (We can also add a fallback later.)InputBehaviorsCommonwhich essentially does whatInputBehaviorsdid before, but using the following callbacks it receives as props:commitFormState;parseNewValue;validateNewValue— with an optionalsetInputMessagesargument.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\ApiLayoutControllerfor now, and save it to the appropriate slice inonquerystarted.Comment #10
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:
Follow-up issues opened:
Comment #12
larowlanI think this is ready for review now
Comment #13
bnjmnmIt looks like this introduces some changes to Checkbox and Radio elements within the context forms. Are these purely behind-the-scenes changes that result in the same experience, or is there new or altered functionality? If it's the latter, there should probably be tests to ensure the functionality does not regress. Due to the size of this MR and its ability to make rebases painful, I think it would be fine to take care of those tests in other issues as long as those issues are properly prioritized.
Comment #14
balintbrewsRe: #13
Very good catch! Those are introducing new functionality — only used by the page data form for now. They are not covered in
prop-types.cy.js, because no prop type is using those elements. I'll create the issue to write tests.Comment #15
balintbrewsComment #16
bnjmnmI approved this, but with the assumption that the thread with my final bit of feedback about moving
shouldSkipJsonValidationis addressed.Comment #17
balintbrewsComment #19
effulgentsia commentedThis looks great. @hooroomoo also reviewed this MR, so crediting them.
Comment #20
effulgentsia commentedI tried to merge this to 0.x, but looks like it needs manual conflict resolution with recent 0.x commits.
Comment #22
balintbrewsThanks, everyone, for the great reviews, the collaboration, and the patience with this issue! 💫
Comment #23
wim leersFYI: Posthumous review of this MR posted, and summarized at #3495752-19: Send page data to Drupal for storage in auto-save store.