Overview

#3469235: Make "page data" tab in right sidebar work introduced the "Page data" tab in the right sidebar with the node form rendered via the Semi-Coupled theme engine. That initial implementation didn't cover connecting the form values to the application state —the Redux store— which is also a prerequisite of supporting undo/redo actions in that form.

Proposed resolution

  1. Connect the page data form values to the Redux store.
  2. Support undo/redo actions for form value changes.
  3. Reduce the displayed fields to the following:
    • Title
    • Menu settings
    • Comment settings
    • Revision information
    • URL Alias
    • Authoring information
    • Promotion options
    • Published/Unpublished

User interface changes

Only a predefined set of fields is shown in the page data form.

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

balintbrews created an issue. See original summary.

balintbrews’s picture

I discussed next steps with @bnjmnm on a call last week. We agreed on the followings:

  1. We will make use of FormStateContext implemented by @/components/form/components/Form to provide the information of what form is being rendered: currently this will mean the component props form or page data form.
  2. InputBehaviors can 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.
  3. We will not change how the Twig to JSX mapping is done, so reverting my previous commits, twig-to-jsx-component-map.js will stay unchanged.
  4. All mapped components will be split into two (or more) components, where a component with its name prefixed with Drupal will render another component (or components) which are purely presentational. The Drupal-prefixed component will wrap the component(s) it renders in InputBehaviors. 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 InputBehavior does 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.

Overview of InputBehaviors

larowlan’s picture

Just 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.

balintbrews’s picture

Good 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.

balintbrews’s picture

balintbrews’s picture

@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. 😌

balintbrews’s picture

Status: Active » Needs work

Draft code is up showing the idea:

  1. InputBehaviors wraps the input in one of InputBehaviorsComponentPropsForm or InputBehaviorsEntityForm. (We can also add a fallback later.)
  2. These wrappers both wrap the input in InputBehaviorsCommon which essentially does what InputBehaviors did before, but using the following callbacks it receives as props:
    • commitFormState;
    • parseNewValue;
    • validateNewValue — with an optional setInputMessages argument.

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\ApiLayoutController for now, and save it to the appropriate slice in onquerystarted.

balintbrews’s picture

@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:

  1. Reroll to incorporate changes from #3492511: Move form state into the global store;
  2. Sending page data to backend;
  3. Test coverage;
  4. Code clean-up.

Follow-up issues opened:

larowlan changed the visibility of the branch 3487484-page-data-form-redux-mid-merge to hidden.

larowlan’s picture

Status: Needs work » Needs review

I think this is ready for review now

bnjmnm’s picture

It 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.

balintbrews’s picture

Re: #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.

balintbrews’s picture

bnjmnm’s picture

Assigned: bnjmnm » Unassigned

I approved this, but with the assumption that the thread with my final bit of feedback about moving shouldSkipJsonValidation is addressed.

balintbrews’s picture

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

This looks great. @hooroomoo also reviewed this MR, so crediting them.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

I tried to merge this to 0.x, but looks like it needs manual conflict resolution with recent 0.x commits.

  • balintbrews committed 73641431 on 0.x
    Issue #3487484 by balintbrews, larowlan, bnjmnm, hooroomoo: Save page...
balintbrews’s picture

Status: Needs work » Fixed
Issue tags: -Needs reroll

Thanks, everyone, for the great reviews, the collaboration, and the patience with this issue! 💫

wim leers’s picture

FYI: Posthumous review of this MR posted, and summarized at #3495752-19: Send page data to Drupal for storage in auto-save store.

Status: Fixed » Closed (fixed)

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