🇬🇧United Kingdom @f.mazeikis

Reviewed and approved. It seems MR still needs approval from @tedbow or @effulgentsia.

tedbow → credited f.mazeikis → .

f.mazeikis → made their first commit to this issue’s fork.

balintbrews → credited f.mazeikis → .

f.mazeikis → made their first commit to this issue’s fork.

Looks good, approved

Re-ran pipeline and it's now passing. Code looks good too - certainly an improvement, even if we still need to come back to this once we have solutions for datetime and image (and entity references in general) props handling.

I've created #3475584 📌 [PP-1] Add support for Blocks as Components Active to start working on "Blocks as Components" requirement. It's based on @larowan work started in MR68 (closed). It's limited scope, focusing only on blocks for now and using the "Component Source Plugin" approach, as per #12 🌱 [META] Support component types other than SDC Needs work .

I wasn't marking it as blocking as it looked as if the issue would be merged anyway and we could have started this issue while the work continued on #3469609 📌 Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs Fixed . And it is merged now.
I've moved most of the code from MR68 (closed) by @larowlan and reached out to him on slack as I couldn't retain his commit signature.
This is not a working commit and will be used as starting point.

f.mazeikis → created an issue.

All the feedback from me and Dave has been addressed, moving to RTBC

Additional docs, ADRs and context is really neat!
Posted some nitpicks and minor feedback about sections that I personally found difficult to follow.

Your self-review questions on MR and contextualisation of commits was really helpful.
There was a lot of context and information to go through, so it took some time.
But at the end I feel like I know much better what is happening in that MR and in general with Props/Shapes/JsonInterpreter related part of backend.
Hence the lack of nitpicks - it is a significant improvement to clarity when it comes to namespacing, docs and inline docs.

Well, the MR is mostly clarifying, removing dead code and updating docs. I don't have any nitpicks 😅

My only comment is that perhaps we should file an issue for #note_374141, just so we don't lose track of that it still needs to be addressed.

wim leers → credited f.mazeikis → .

Applied suggestions, converted controller into a service, added comments.

f.mazeikis → created an issue.

f.mazeikis → created an issue.

Addressed additional feedback, waiting on more feedback on 2 unresolved threads in MR.

Bonus idea: a "Refresh" button that calls ::clearCachedDefinitions() + ::getDefinitions(), to force a refresh :)

This already happens on every page load for the "Disabled components" page. Would you prefer that not to happen and introduce a separate "refresh" button to trigger this behaviour manually?
Other than that, I've addressed (or at least replied to) the feedback on the MR.

I think I've addressed all of the feedback.

Pushed the changes so far, don't want to polish this too much before we agree it's going in the right direction.

wim leers → credited f.mazeikis → .

f.mazeikis → made their first commit to this issue’s fork.

+1 to using enable()/disable() for now
Implementing a new requirement "must not be obsolete" for auto-adding is simple enough
Auto-delete needs in-use tracking, as per #3 📌 New component requirement: obsolete SDCs must not be auto-added, but must be auto-updated Active .

f.mazeikis → created an issue.

We’ve got this pretty close to done, but it is still failing Cypress test. Specifically, due to Shoe Button (components/simple/shoe_button/shoe_button.component.yml) component missing. It is not being auto-generated as there’s no StorablePropShape for it’s icon prop.
The way I see us resolving this:

• Removing/updating cypress test (I originally was thought it was testing a feature we need, but @wim-leers tells me otherwise)
• Prioritising this might resolve #3467890 📌 [later phase] Support `{type: object, …}` prop shapes that require *multiple* field types — also: nested components/component reuse Postponed , but that issue was deprioritised and would take quite a while
• We could consider adding another hardcoded match for icon shape prop in Drupal\experience_builder\SdcPropJsonSchemaType::computeStorablePropShape as we are already doing for image (L243), but it also needs #3467890 📌 [later phase] Support `{type: object, …}` prop shapes that require *multiple* field types — also: nested components/component reuse Postponed

I'll proceed with removing or modifying the failing Cypress test.

Wim Leers → credited f.mazeikis → .

Wim Leers → credited f.mazeikis → .

f.mazeikis → created an issue.

Wim Leers → credited f.mazeikis → .

Reviewed, re-read and approved. Nice amount of details in there, the Terminology section alone is worth it's weight in gold. I see @wim-leers is already responding to @longwave threads, so we just need @tedbow to have a look.

Postponed until blocker 📌 Add fieldInstanceSettings to (Candidate)StorablePropShape Active is resolved.

f.mazeikis → created an issue.

Approved.

Approved - that’s a really neat solution until upstream changes happens

Wim Leers → credited f.mazeikis → .

f.mazeikis → made their first commit to this issue’s fork.

Wim Leers → credited f.mazeikis → .

Wim Leers → credited f.mazeikis → .

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

Wim Leers → credited f.mazeikis → .

f.mazeikis → made their first commit to this issue’s fork.

f.mazeikis → created an issue.

Wim Leers → credited f.mazeikis → .

Wim Leers → credited f.mazeikis → .

Wim Leers → credited f.mazeikis → .

Pushed component entity definition + schema + ensuring dependency of SDC provider is added to Entity on creation.
Added very simple unit test to ensure config entity creation works.
Added forms, routes and menu items/actions for creating/editing/deleting entity - it's all based on existing core logic, nothing fancy, just so FE people could easily interact with new entities.
When creating component SDC selection is presented in a select dropdown, grouping SDC's by their provider.

Wim Leers → credited f.mazeikis@gmail.com → .

Wim Leers → credited f.mazeikis@gmail.com → .