- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Congrats!
Unpostponed ✨ [PP-1] Include component props form in undo Postponed and ✨ [PP-1] Update preview automatically when component props are updated Postponed .
Thanks for that epic screenshot 🤩
- 🇺🇸United States bnjmnm Ann Arbor, MI
Good feedback, all addressed. This is going in.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Working to get this issue's status/relevance updated, but blocked on @lauriii's confirmation at #3463999-7: [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria → .
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Follow-ups:
- #19.1: 📌 [later phase] [PP-1] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed (and I realized it had a blocker itself: ✨ [later phase] PropShapes' JSON schema must match exactly with FieldType's storage format — what if you want to use another FieldType? Active ), and then the 3 at the bottom of #17:
- 📌 [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria Active
- 📌 [PP-2] Remove Component config entity's add/edit form Postponed
-
📌
[PP-1] Introduce `FieldTypeStorageForJsonSchemaDefinition` plugin type
Postponed
Bonus follow-ups: 📌 Component config entity should validate that the SDC actually (still) exists Active + 📌 [PP-2] Add ComponentAuditabilityTest Active .
I'll continue working on this MR on Monday. Now: time for blog posts :)
Automatically closed - issue fixed for 2 weeks with no activity.
- Issue created by @Wim Leers
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
FYI: this will also unblock ✨ [PP-1] Update preview automatically when component props are updated Postponed — but unlike 📌 [PP-2] Redux sync on ALL prop types, not just ones with a single [value] property Postponed , that has no additional blockers, so it will become immediately unblocked upon this landing.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Wow, that looks much better! 👏
I'd even use the word impressive — it's nice to see that when we use Symfony's Validation component "as intended", that it is so much clearer than if there's Drupal's layer of abstraction (either config schema or Entity/Field/Typed Data) in between 🤯
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
It will be much easier to implement when […]
💯 — plus, then there should be a broader spectrum of "widget/form complexity" — because
StringItem
's widget is on one extreme, andImageItem
's is pretty much on the other extreme 😅Thanks for creating 📌 [PP-2] Redux sync on ALL prop types, not just ones with a single [value] property Postponed , that helps clarify the intentionally-out-of-scope parts of this issue. I just updated the title to make it clear what the difference is between this issue and its follow-up 👍
That would of course need to happen, and I'll make it more explicit in the code.
👍
but if thats concerning it's NBD to do it all here and postpone this on #3463583
Hell no! 😄 We want to iterate in small pieces, and now that there is a follow-up and you'll make the code/docs more explicit, I have no hesitation to land this 😊 Especially because it will surely help discover additional next steps on the front end.
I'm not sure if the solution will be a more clever parsing of the Drupal selector or if we figure out server side and provide that info in an additional attribute.
Interesting! I think that could totally work for most field types. 👍
Given only docs (including a
@todo <follow-up issue URL>
comment) and a screenshot are missing, and some minor remarks on the MR, I pre-emptively approved the MR, so you can address those bits and merge this MR, to end your week on an epic note 😊 - @hooroomoo opened merge request.
Is there any documentation about how `#config_target` should/can be used?
- 🇺🇸United States tedbow Ithaca, NY, USA
@Wim Leers I changed this to use symfony/validator constraints. I think this is better.
I still need to add some test cases for missing keys and type errors but this logic should already be covered by the constraints.
So I have still have work on it. If you want to just wait until I am done, I can just continue working on it. But if you want to look at the approach feel free
- 🇺🇸United States bnjmnm Ann Arbor, MI
In other words: what about field types with multiple props, such as ImageItem?
That would of course need to happen, and I'll make it more explicit in the code. It will be much easier to implement when the all-props component can be added as a page component in 📌 Create a component for testing form backend + frontend integration Active (or whatever issue is blocking that one).
I'm not sure if the solution will be a more clever parsing of the Drupal selector or if we figure out server side and provide that info in an additional attribute. I feel like what is here is worth adding now and addressing the different prop shapes in a separate issue - but if thats concerning it's NBD to do it all here and postpone this on #3463583. - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Detailed review on the MR 🏓
I could see this being sufficient for now, to meet 🌱 Milestone 0.1.0: Experience Builder Demo Postponed: needs info goals.
But I don't see yet how this can fulfill all needs, so at minimum, this AFAICT needs follow-ups. How many follow-ups probably depends on what I missed aka what you can provide a solid answer to :)
Finally, some documentation, even if it's sparse, would help grok this and clarify next steps —
InputBehaviors
should explain what assumptions it makes WRT widgets'name
attributes matching the Field Type property structure exactly, those values not needing transformations, how/if either of those could evolve, etc. - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Discussed #17 with @lauriii. His replies:
- Yes, but needs follow-up to devise a UX to allow existing
StaticPropSource
s using a previous field type+widget to update to the new one. - Yes
- Yes
He agreed with the 3 follow-ups proposed at the bottom of #17.
We also discussed #18, and he prefers the "alternatively" — so repurposed that 👍
P.S.: Note that this also blocks 📌 Create a component for testing form backend + frontend integration Active .
- Yes, but needs follow-up to devise a UX to allow existing
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
But yeah once you have unlocked slots in more than one view mode there is a lot to deal with. One way to deal with it would just be to prevent it everywhere except full/default.
Interesting proposal! WDYT, @lauriii?
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Oh, and once this lands, 📌 [PP-1] Media Library integration Postponed becomes trivially unblocked: it'll only require:
- new configuration to allow declaring a preference for
image
vsmedia
(which oddly would not be possible through a newexperience_builder.settings.yml
config, only through a new config entity, because simple config cannot have module nor config dependencies) - updating
\Drupal\experience_builder\SdcPropJsonSchemaType::findFieldTypeStorage()
to respect that configuration
Those 2 things would be in an MR for that issue which I could start and then the front-end team could continue.
Alternatively, I could repurpose ✨ Hardcode image props to use the media widget temporarily Active for that, so that #3454173 can stay focused on the bigger picture, of making that media library widget actually work (well).
- new configuration to allow declaring a preference for
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
The
\Drupal\experience_builder\Form\ComponentEditForm
UX became simpler:Discussion with @lauriii
Yesterday morning, prior to writing #15, @lauriii and I met and discussed this problem space in detail.
The key outcomes of that discussion:
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
examples
array must not be empty, and XB will use the first one. ⇒ Removes the need for 25% of what theComponent
config entity provides. - @lauriii was heavily leaning towards "no UI" and "logic with an alter hook".
- Tricky: complex shapes, like
json-schema-definitions://experience_builder.module/image
— those would need a plugin system to provide the appropriate field type + field storage.
This issue: current MR state and current consequences
This issue proves it's possible to generate the default
field_type
,expression
andfield_widget
value for each prop. ⇒ Removes the need for 75% of what theComponent
config entity provides.This issue did implement it all as logic (i.e. with nothing stored as a config entity — see my comment #15 for why that turned out to be trickier than I thought when I created this issue), with a TODO for an alter hook.
As demonstrated in #16, this means the scope for 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed changes to only having to worry about matching existing field instances that can be adapted to match the SDC prop's enum values.
Questions for @lauriii prior to landing this MR
- Is it acceptable for the logic that determines which field type + widget to use to change and hence newly created instances of this component to potentially use a different field type than the pre-existing component instances?
- SDC supports default
*.component.yml
-defined values. But that won't work for e.g. a default image. Is it okay for XB to layer on support for that by auto-discovering apropname.(gif|png|jpg|…)
file in the SDC's directory? 🤔 (Will add this to 📌 [SPIKE] Comprehensive plan for integrating with SDC Needs work , to potentially add that to upstream SDC.) - Is it acceptable that the default value sourced from a component's
examples
cannot be overridden on a per-site basis?
I think the answer to the first question is "yes". (Note that every stored
StaticPropSource
is self-contained: the user will continue to be able to edit it, even if it's a completely different field type.)If the answer to the second and third questions is "yes", then we can in principle remove the
Component
config entity. But that would introduce a huge risk: it'd make exporting/sharing configuration (in the future: 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active , but now:config/optional/field.field.node.article.field_xb_demo.yml
, which has config dependencies on theComponent
config entities it uses) brittle.Therefore I think it'd be preferable to keep
Component
config entities under the hood — without showing any of that in the UI. I think that would also keep the door open for supporting per-site default values in the future, plus it will likely help with 🌱 [META] Support component types other than SDC Active .If you agree, then I'll create three follow-up issues:
- one that generates
Component
config entities for all discovered SDCs 1) upon installing XB, 2) upon installing a new module or theme - one to remove
\Drupal\experience_builder\Form\ComponentEditForm
after that auto-generation is happening - one for computing the appropriate field type + storage for
$ref
-defined prop shapes
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
- 🇫🇷France andypost
The new native
#[\Deprecated]
attribute in PHP 8.4 can automatically throw deprecation messages, so external libraries can give unexpected deprecations - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
I started with a test that gathers all unique SDC prop schemas. Then I started writing logic to automatically suggest the sane core-provided field type choice.
In doing so, I started treading on the territory of #3456008 — and I believe that we now should not do that issue anymore, at least not in its original scope: 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed .
I am now thinking that not using a config entity type might actually be the only viable choice, because the config entity being proposed here only is viable if there is a good way to transform a given
SDC prop schema
into aPropShapeInput config entity ID
— because that's how we could achieve sane dependencies.The only reason that worked in the original proposal, is because HEAD does not yet support the
{type: string, enum: …}
use case, which is AFAICT widespread among SDCs. Once that is considered, the only way to generate a config entity ID is by hashing the enum.The issue title + summary need a drastic update. See the new test class, which has 3 test methods that build on top of each other. I'll continue this MR tomorrow and will update this issue as well as 🌱 [META] Early phase back-end work coordination Active .
Automatically closed - issue fixed for 2 weeks with no activity.
- @wim-leers opened merge request.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
Automatically closed - issue fixed for 2 weeks with no activity.
- First commit to issue fork.
- @bnjmnm opened merge request.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@lauriii is not yet convinced, but he's not available to discuss it further, so to A) get this moving, B) ground the conversation with @lauriii more, getting started on this nonetheless.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
(To elaborate on #4: @hooroomoo landed that in ✨ Create UI scaffolding for the primary panel (left sidebar) Needs work .)
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
This is making a pretty drastic change that is not captured in the issue summary: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... — in that comment I explain why we should revert that and leave such a change for 🌱 [META] Support component types other than SDC Active to make.
I also found 2 critical gaps in the test coverage:
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
… and a number of additional questions.
- 🇺🇸United States phenaproxima Massachusetts
📌 Add validation constraints to language.content_settings.*.* Needs work landed!
- 🇬🇧United Kingdom jessebaker
Assigning to @hooroomoo as they have been working on the left hand menu so far.
Sorry hooroomoo, it looks like there are some iterations/updates required to the menu work you did now that the designs have progressed further.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Updated #3462310-10: Component edit form: make form elements match design → and I see @bnjmnm already started working on ✨ Contextual form values need to be integrated with Redux Active 🚀
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
📌 [PP-1] Evolve component instance edit form to become simpler: generate a Field Widget directly Postponed landed last night, which makes it easier to work on this :) I see @bnjmnm pushed a first WIP commit to the issue fork already — yay, glad to see this moving 👍
- First commit to issue fork.
- 🇺🇸United States phenaproxima Massachusetts
📌 Fix Block config entity type config schema violations: weight, property Postponed is in!
-
bnjmnm →
committed 9b971118 on 0.x authored by
Wim Leers →
Issue #3461422 by Wim Leers, bnjmnm, tedbow: Evolve component instance...
-
bnjmnm →
committed 9b971118 on 0.x authored by
Wim Leers →