Allow specifying default props values when opting an SDC in for XB

Created on 4 June 2024, 22 days ago
Updated 20 June 2024, 6 days ago

Problem/Motivation

Blocked on ๐Ÿ“Œ "Developer-created components": mark which SDCs should be exposed in XB Active + ๐Ÿ“Œ Add ::calculateDependencies() to ComponentTreeItem Active .

Once we have a minimal config entity to opt in an SDC for use in XB (see ๐Ÿ“Œ "Developer-created components": mark which SDCs should be exposed in XB Active ), we need the ability to specify default values for any SDC prop โ€” regardless of whether that SDC prop had an examples key-value pair in its *.component.yml file.

(This was descoped from #3444417 โ€” see #3444417-11: "Developer-created components": mark which SDCs should be exposed in XB โ†’ .)

Related discussion: #3444424-4: [META] Configuration management: define needed config entity types โ†’ .

Steps to reproduce

N/A

Proposed resolution

What does that look like?

  1. The data should be stored in the config entity introduced by #3444417 in exactly the same way as ComponentPropsValues does for the XB field type.
  2. โ€ฆ but in this first issue we would not allow sourceType: dynamic, because that would complicate this issue/MR: it'd introduce the concept of context requirements. For now, we want these default props values to be context-free/context-independent. That also means ruling out adapted sources for now. IOW: only StaticPropSource (sourceType: static:โ€ฆ) is allowed.
  3. A static prop source uses a field type. For many field types, the above won't be an issue, because the data is entirely self-contained (e.g. the string, link etc. field types). But for others, it's more challenging:
    • The text field type uses a Text Format. That is a config entity too: filter.format.*. Therefore, as soon as a text field type is used for a default value, a config dependency on the relevant filter.format.* must be declared.
    • More challenging still: entity references. Not every entity reference ought to be allowed. Only entity references that target a File entity should be allowed, at least initially. Because files can reasonably be serialized into the exported config, but the same is not true for arbitrary entity refernences.
      This is where @Felix' prior PoC MR comes in, to allow exporting/syncing even default props values that rely on e.g. images.

I defer to @Felix to decide how to split up point 3 above: I think it might be best to not do any of that in this issue, and instead restrict this issue to only supporting the field types that have a single scalar property. Then the text format example could be a first follow-up, and the image/file example could be a second follow-up.

Remaining tasks

TBD

User interface changes

TBD

API changes

TBD

Data model changes

TBD

โœจ Feature request
Status

Needs work

Component

Data model

Created by

๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Wim Leers
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    FYI: @larowlan also observed the need for default values for components independently, at https://git.drupalcode.org/project/experience_builder/-/merge_requests/2....

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Status changed to Active 13 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Looks like ๐Ÿ“Œ Add ::calculateDependencies() to ComponentTreeItem Active is largely done, so unpostponing: work here can begin with just building on top of that MR, while the test coverage for that MR is sorted out. ๐Ÿ‘

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    ๐Ÿ“Œ Add ::calculateDependencies() to ComponentTreeItem Active is in!

    Let's restrict the issue scope like I proposed:

    I think it might be best to not do any of that in this issue, and instead restrict this issue to only supporting the field types that have a single scalar property.

    Small iterations that can land quickly result in more visible progress and fewer conflicts ๐Ÿ‘

    I'll update the issue summary and will create the follow-ups, you just focus on the implementation work, Felix ๐Ÿ˜„

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom f.mazeikis@gmail.com Brighton

    f.mazeikis โ†’ made their first commit to this issueโ€™s fork.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    To clarify the scope/intent of the issue, the attached patch showcases the additional configuration I want to see this issue add support for, and for which a UI should be built.

    The result will be that components, when dragged onto the canvas, will start out with these default values, which will ensure they'll be visible from the start.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
    +++ b/config/schema/experience_builder.schema.yml
    @@ -13,6 +14,61 @@ experience_builder.component.*:
    +                Choice:
    +                  # @todo Currently, FieldForComponentSuggester assumes an entity context. We need to extract a new FieldTypeForComponentSuggester service out of that *first*. Until then, hardcode a list of possible field type choices.
    +                  - string
    +                  - uri
    +                  - file
    +                  - image
    

    Created ๐Ÿ“Œ FieldForComponentSuggester must be able to work without an entity context Needs review to unblock this โ€” ready for review.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom f.mazeikis@gmail.com Brighton

    @wim-leers I've got a few questions I'd like to clarify about the scope of this issue:
    UI - do imagine this being multi-step process form along the lines of โ€œselect component option -> ajax callback loads field type selection -> another ajax callback loads field widget selection -> selecting that results in another ajax callback that appends correct widget formโ€, rinse and repeat for each required prop?
    Is the UI at this stage supposed to be โ€œniceโ€, or just โ€œworkingโ€? Thinking how much time is worth spending on trying to build nice(ish) UX for this using Drupal forms and ajax callbacks.
    For entities saved from array of values (tests/config import/etc) - should validate if the โ€œcombination of valuesโ€ is actually valid? For example, if โ€œfield typeโ€ and โ€œfield widgetโ€ values are actually compatible?
    As per your example - "defaults" property in schema is not marked as nullable (not sure if that's the correct schema constraint) - this means that for all component config entities "defaults" will be made mandatory, right?
    Do we need to update `src/Controller/SdcController.php` to include default values in `/xb-components` output as part of this issue?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    โ€œJust workingโ€ for sure, it can totally be clunky.

    Iโ€™d encourage using #states to keep it as simple as possible in this first iteration.

    Do not bother with validating field type and widget compatibility for now, just add a @todo. Only validate what is easy to validate, which is what I already added!

    Nullability: excellent point! I forgot that. My thinking: required SDC props require default values, others donโ€™t. But letโ€™s start out with them all being optional.

    Exposing defaults via the controller: need to look closer, will do.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    It's not clear to me whether this is for:

    - a default value, which would pre-populate the widget for a component, and be saved as entity data.

    - a 'show if empty' value (like a fallback image) which is pulled from the layout/xb configuration only when there is no entity data. (views 'no results' behaviour and argument defaults are somewhat similar-ish).

    If it's the latter, or even both (?), the terminology might need changing?

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    #13: I'd expect it to be the former.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    If a component is backed by field data, which takes precedence? The default values for the field, or the default values for the SDC?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    It's indeed the former.

    Would be a better name?

    But โ€ฆ isn't that exactly the same behavior as the default value you configure for a FieldConfig (field.field.*.*.*: default_value)? So why is the name confusing?

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    #15: I'm pretty sure that it would be both easier and would make more sense from UX perspective to use fields default value. The fields default value is specific to the current context, meaning that it could have a value that is more specific than what the component has. The components prop would have a PropExpression as a value so at least in my mental model it should just render the default value from the field that is powering it. I'd imagine in this scenario that if there's no default value for the field, that the default value for the prop would not take effect, because otherwise you run into challenges in the scenarios where you want to be able to leave the value empty.

    If we were to do wise versa, we would somehow have to bubble up the default value from the component prop to the field. That seems pretty unexpected consequence to me.

    This is all just my hypothesis, we'd definitely do user testing on this later once we have a version of this implemented.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    But โ€ฆ isn't that exactly the same behavior as the default value you configure for a FieldConfig (field.field.*.*.*: default_value)? So why is the name confusing?

    If it's a default value, then it's not confusing, my 'both' meant if there's an option between default value and 'show on empty' somewhere, which sounds like it's not the case.

    However, also see the question in #15 because I think that might be a source of confusion.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #15: An SDC does not have default values. It's got examples, and they're optional. If that were required, we would indeed not need this issue/MR to define default props values!

    But we'd still need this issue to specify which field type/widget should be used by default when using/placing an SDC, to populate its props.

    To expand what @lauriii wrote in #17:

    1. opt in an SDC to XB โ€” that creates this config entity, that with this issue in, will also require you to specify a default field type + default field widget + default value (a StaticPropSource) for each SDC prop
    2. consequence: when placing this SDC/component, it is guaranteed to have a visual representation
    3. consequence: when placing this SDC/component, you'd be able to edit it using a choice of field type + widget that the Site Builder made, removing the need for the Content Creator to choose it
    4. later, by ๐ŸŒฑ Milestone 0.2.0: Experience Builder-rendered nodes Postponed , the Content Creator will be able to not use a statically defined value (a StaticPropSource) and switch it to use one of the values on the host entity (e.g. the article's image, the article's title โ€ฆ โ€” a DynamicPropSource โ€” which is basically only a PropExpression to retrieve that value, as mentioned by @lauriii) if there is a field in the content type that matches the shape of the SDC prop
    5. much later, the Site Builder would be able to specify in the content type template (see ๐ŸŒฑ [later phase] [META] 7. Content type templates โ€” aka "default layouts" โ€” affects the tree+props data model Active ) that a component placed in the default layout has a prop populated by a field (a DynamicPropSource).

    IOW: this issue is only a stepping stone to unblock the UI demo targeted for ๐ŸŒฑ Milestone 0.1.0: Experience Builder Demo Active . Because it provides a default that is independent of (content entity) context, it only needs to support StaticPropSource. Once we have a content type template, you'd be able to use a DynamicPropSource. See EndToEndDemoIntegrationTest for a walkthrough of how that works. (Docs for that coming later.)

    If we were to do wise versa, we would somehow have to bubble up the default value from the component prop to the field. That seems pretty unexpected consequence to me.

    Agreed that would not make sense.

    Which is why โ€” as explained in the bullets above โ€” we'd never have a DynamicPropSource in this component config entity that is being worked on here: because there is no (entity) context when opting in an SDC using this component config entity.
    DynamicPropSource will be available as soon as there is an entity context, which is true in two places: when creating a content entity, or when defining the default layout/content type template (see ๐ŸŒฑ [later phase] [META] 7. Content type templates โ€” aka "default layouts" โ€” affects the tree+props data model Active ).

  • Status changed to Needs work 6 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    The default value being stored here is not a valid (default) value for a field. Try adding a "plain text" field to e.g. the Page node type, and specify a default value. This is what ends up in the config:

    default_value:
      -
        value: 'this is the default'
    

    i.e. [0 => ['value' => 'this is the default']]

    The logic in the current MR tries to be clever and stores only the first delta's main property:

    defaults:
      text:
        field_type: string
        widget_type: string_textfield
        default_value: this is the default
        expression: โ€ฆ
    

    That will not work when we eventually add support for multiple-cardinality fields (and hence also defaults), which will be relevant for SDCs that have props that need lists of values.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    That bug hid my excitement โ€” it's really cool to start see this working! ๐Ÿ˜„

    Note that this is a hard blocker to make e.g. the default component work in the UI, because without a default value set for its required image prop, it fails/refuses to render โ€” give ๐Ÿ“Œ Connect client & server, with zero changes to client (UI): rough working endpoints that mimic the UI's mocks Needs review a try to see that happening, that is what this will unblock! ๐Ÿš€

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I fixed the bug in #20.

    ๐Ÿž Remaining bug: the expression should be that for the chosen FieldTypePropExpression, because that's what's necessary to construct a StaticPropSource. It should not be the SDC prop expression. So for example, if you chose the string field type, it'd be โ„น๏ธŽstring_longโŸvalue.

    Missing (I did not specify all of these in the issue summary โ€” but reviewing it made me realize this):

    1. Form UI: making required SDC props require to have default values
    2. Form UI: matching that requiredness: only required SDC props should have #open โ‡’ TRUE
    3. a new Component::getDefaultStaticPropSource(): StaticPropSource method that converts that default value into an actually usable StaticPropSource! ๐Ÿ˜„
    4. Tests: expand ComponentValidationTest
    5. Tests: new ComponentTest kernel test to verify ::getDefaultStaticPropSource() works as expected
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Note that updating any API logic that powers the UI is out of scope here, for that we have ๐Ÿ“Œ [PP-1] API: update /xb-render-component/{component_id} to use Component config entity's default values Postponed as a follow-up ๐Ÿ˜Š

Production build 0.69.0 2024