Implement saving block settings forms

Created on 5 December 2024, about 2 months ago

Overview

In πŸ“Œ Define `props` in the context of Block components Active we enabled block component settings forms in the UI, but they are not yet functional. This issue exists to complete that work and make block settings changes in the UI propagate to the back end including data storage and preview.

Block components have no slots, but they can have "block settings". These are used to configure the block and (in most cases) affect its output in some way. Examples are the number of levels of hierarchy to show in a menu block, or checkboxes to control which elements are displayed in the site branding block.

Block settings are similar to, but not the same as, SDC props.

Proposed resolution

?

User interface changes

πŸ“Œ Task
Status

Active

Version

0.0

Component

Data model

Created by

πŸ‡¬πŸ‡§United Kingdom longwave UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @longwave
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    This needs πŸ“Œ Define `props` in the context of Block components Active to land first.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    There is work in πŸ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active that will make this easier. Should we postpone on that too?

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    I'm not opposed to that at all, but @longwave, @jessebaker and others felt that πŸ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active was too broad in scope to be actionable. If you managed to act on it anyway, all the better! πŸ˜„

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    I learned enough React to be able to hack something together here that seems to work. Block settings are a hybrid of SDC component forms (as they update the model) and page data forms (as they use standard Drupal forms), I think inputBehaviors needs some more refactoring to figure this out to how split this up in a better way.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Ran out of time today, but compared the component inputs form HOC against the block one and I think we should be able to make them one and the same. Because the form state and these input behaviours are locked up in the compiled FE build, any deviation that is per source component is going to make things difficult for contrib (and a layout builder/paragraphs BC layer).
    Ideally we can leverage the SimpleComponent/SDCComponent differentiation in #3493941 and then defer any other nuance between Source plugins to pure PHP. That leaves the door open for Layout plugins (LB), paragraph plugin and anything else from contrib

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Per @larowlan's latest MR comment.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Been going back and forth to try and figure out the best way forward here. The problem is that the SDC version of the form presents field widgets, and these have keys from field widgets, e.g. heading[0][value], which we then map into props. Block forms (and any other non-widget form I guess) just has plain keys, e.g. level. But we can't make assumptions, because there might be complex block forms that have structures that look a bit like widget forms, but aren't - so we do need to distinguish between them somehow - but right now these Form API structures have specific assumptions made about them.

    As far as I know there is no requirement to have block forms have dynamic props, we can assume static values. But not sure about other component source plugins, they might still want to support dynamic props?

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    In https://git.drupalcode.org/project/experience_builder/-/merge_requests/448 I've tried to remove those assumption - but it will likely need to come back in some form by way of transforms in πŸ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active but ideally it can be controlled from PHP

    For ✨ Save page data form values in application state with support for undo/redo Active we were able to remove the knowledge of that nesting and let Drupal sort it out - i.e. we store the page values keyed by the name attribute for their respective input (e.g. 'title[0][value]' and then that is transformed back into the nested format Drupal side.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    It's green!

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    This looks ready to me in its current form. There's a followup for the reunification.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    This issue exists to complete that work and make block settings changes in the UI propagate to the back end including data storage and preview.

    (Emphasis mine.)

    The preview part is working.

    The storage part is not. It's only auto-saving (to ephemeral storage), not really saving (to an XB field or the PageTemplate). Which means it's not really stored, nor passing validation.

    Besides that, I am concerned about:

    1. the deviation from the previously established terms "explicit inputs" and "implicit inputs": https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... β€” block settings are explicit inputs for that the "block" component source. As captured by this bit in the issue summary:
    2. the (undocumented) meaning of supportsProperties and supportsDynamicProperties: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...

    Perhaps it's worth meeting on Monday morning about this, @longwave, to figure out the naming and docs? Happy to write those together!

  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    It's only auto-saving (to ephemeral storage), not really saving (to an XB field or the PageTemplate). Which means it's not really stored, nor passing validation.

    How are you determining that without ✨ [PP-1] Implement the "Publish All" button Postponed being done? Are you testing the publish step some other way?

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    IDK who/when/where this happened but there still is a β€œPublish” button next to thr β€œReview changes” dropdown πŸ˜…πŸ™ˆ

  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    Oh, right, I forgot that button was still there. So originally, that button implemented direct-to-entity-save (without going through autosave), so that we could work on issues having to do with saving entities in parallel with figuring out how we wanted to handle autosaving.

    Once we got autosaving working, I think we opened an issue to change this Publish button to not send the entity data in the request body, but instead to just send the autosave key (or the info needed to figure out the autosave key), and the server-side controller to save from the corresponding autosaved data instead of from the request body. However, I forget if we completed that issue.

    So, if that is in fact how the Publish button now works, then I do think it makes sense, as either part of this issue or as a followup that can proceed right after this issue is merged, to make sure that block settings get validated and transferred correctly from the autosave data to an entity revision. Because presumably the work to do that would be exactly the same as what will get triggered by the "Publish All" button once that's in.

    However, if the Publish button is still working the old way where it's publishing from the request body rather than the autosave data, then I don't think it makes sense to make that old way work with block settings. Because once the "Publish All" button is in, we'll remove the "Publish" button.

  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    I've renamed the methods/attribute properties and documented them as follows:

    * @param bool $hasExplicitInput
       *   TRUE if the component source generally expects input from the user. This
       *   could be in the form of props for a single directory component or
       *   settings for a block component. Pass FALSE if the component expects no
       *   input from the user. This can be modified on a per-component basis by
       *   implementing the ::componentHasExplicitInput method in the plugin class.
       * @param bool $hasEvaluatedInput
       *   TRUE if the component stores its input in the form of prop-sources. This
       *   input is stored as a combination of a source-type and expression.
       *   Depending on the source-type it may include additional input including a
       *   value or adapter configuration. A source plugin that passes TRUE here is
       *   also assumed to have explicit input. This can be modified on a
       *   per-component basis by implementing the ::componentHasEvaluatedInput
       *   method in the plugin class.
    
    /**
       * Returns TRUE if the component source expects input from the user.
       *
       * @return bool
       *
       * @see \Drupal\experience_builder\Attribute\ComponentSource
       */
      public function componentHasExplicitInput(): bool;
    
      /**
       * Returns TRUE if the component stores input in the form of prop-sources.
       *
       * @return bool
       *
       * @see \Drupal\experience_builder\Attribute\ComponentSource
       */
      public function componentHasEvaluatedInput(): bool;
    

    And the protected methods on the tree data-type are now componentHasExplicitInput and componentHasEvaluatedInput

    I've also added an e2e test and for clicking publish with a block in the body and expanded test coverage in the client conversion test to ensure this is supported both for nodes and patterns.

    Looking into the last open item regarding validation now.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    This is ready for another look.
    I think we also should consider a general followup to revisit some more uses of 'props' in the codebase including ComponentPropsValues in typed data. I'm happy to keep going on renames here, but it feels like we're getting into out of scope territory and making it harder to review this issue.

Production build 0.71.5 2024