- Issue created by @Wim Leers
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Wim Leers โ credited larowlan โ .
- ๐ง๐ช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 ๐ง๐ช๐ช๐บ
๐ "Developer-created components": mark which SDCs should be exposed in XB Active landed, but a new blocker was identified: ๐ Add ::calculateDependencies() to ComponentTreeItem Active .
- Status changed to Active
13 days ago 8:10am 13 June 2024 - ๐ง๐ช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.
- Merge request !63Draft: #3452397: Allow specifying default props values when opting an SDC in for XB โ (Open) created by Wim Leers
- ๐ฌ๐ง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?
- ๐ฌ๐ง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:
- 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 - consequence: when placing this SDC/component, it is guaranteed to have a visual representation
- 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
- 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 โฆ โ aDynamicPropSource
โ which is basically only aPropExpression
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 - 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 aDynamicPropSource
. SeeEndToEndDemoIntegrationTest
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 ). - 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
- Status changed to Needs work
6 days ago 11:04am 20 June 2024 - ๐ง๐ช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 aStaticPropSource
. It should not be the SDC prop expression. So for example, if you chose thestring
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):
- Form UI: making required SDC props require to have default values
- Form UI: matching that requiredness: only required SDC props should have
#open โ TRUE
- a new
Component::getDefaultStaticPropSource(): StaticPropSource
method that converts that default value into an actually usableStaticPropSource
! ๐ - Tests: expand
ComponentValidationTest
- 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 ๐