Define `props` in the context of Block components

Are block settings conceptually similar enough to props that we can treat them as the same thing?

No, we can't treat them as the same thing.

See #3484669-4: Define how block settings should be presented in the UI → and the comment it links to.

As well as static block settings, do we want dynamic block settings?

AFAICT we don't, because that'd pretty much duplicate the "context" concept that blocks use, but in a more narrow sense:ContextAwarePluginInterface can consume any context, not just that of the host entity. In other words: this would AFAICT "just" be duplicating the equivalent in Blocks-as-XB-Components what \Drupal\layout_builder_fieldblock_test\Plugin\Block\FieldBlock already does … but because then there can be "dynamic resolution of values" at two stages (pre-render-time, in XB, by using DynamicPropSource, and at-render-time, by using ContextAwarePluginInterface), it'd become very confusing?

For example if you had a term field on a node, you might want to map that field value into a block setting so you can embed a list of related articles on the node that automatically corresponds to the selected term.

Hm … 🤔

Can you walk me through how that code flow would work? Is this something like: ? If so, is that not already supported today … through contexts?

OK, so we will need some UI handling for block contexts.

But if we ignore dynamic values and only allow static block settings: why do we have to treat them any differently from static props? They are validatable values, that affect the output of the component.

Clarifying where we currently call things "props" that might not make sense

why do we have to treat them any differently from static props?

Because the sole/whole reason StaticPropSource exists is to reuse Drupal's Field/Typed Data infrastructure to A) store, B) manipulate (through a UI) values for SDC props.

But block plugins already offer a UI, so the "B) manipulate" part is already handled, we only need the "A) store" part. Storing each key-value pair in a block setting in a field seems overkill. And … we do not yet have the ability to store arbitrarily complex values as StaticPropSources! And some block plugins sure do have very complicated structures…

IOW:

1. the explicit inputs for an SDC are "props", and SDC offers no UI for these. (There are no implicit inputs.)
2. the explicit inputs for a block plugin are "settings", and a block plugin does offer a UI for these. (There are implicit inputs too: contexts. → 📌 Handle block contexts Active )

Given that we know we want `props` for SDCs but `settings` for blocks I think we can use this issue to:
1. Ensure that separation is done everywhere following 📌 Add support for Blocks as Components Active
2. Introduce settings forms for blocks instead of props forms for SDCs.

Repurposing this slightly now we know `props` and `settings` are different, we can use this to figure out how to handle `settings` as the related @todos all still stand.

📌 Add support for block derivatives Active is also in, making this more actionable.

As @larowlan already wrote on Nov 5: needs rebase.

I think this issue is critical in helping shape #3467954 — see #3467954-27: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc. → .

FYI, as I was refactoring the Accordion component in ✨ Save page data form values in application state with support for undo/redo Active , I lifted @bnjmnm's awesome solution from this issue to handle <details> outside of vertical tabs: c8d73a03. We will have separate DrupalVerticalTabs and DrupalDetails components which will include AccordionRoot+AccordionDetails and Details, respectively, with a single CSS file that also accounts for animation for both.

I'm happy to help with merge conflicts if and when they need to be resolved. 😊

Related FYI: per #5 + #6 over at 📌 Handle block contexts Active , the implicit inputs of block plugins is out of scope for 0.2. This issue/MR is handling the explicit inputs of block plugins. 👍

Discussed with @bnjmnm and we agreed to split this into two issues: this one, where we allow block settings forms to be displayed in the UI but they are non functional, and 📌 Implement saving block settings forms Active , where we allow the settings changes to take effect.

There is an open question on the MR here about what to do with the SDC props values as they arrive from the front end, but it is not clear how to refactor this until we also have block settings values arriving from the front end, then we can figure out commonalities and move as much as possible into the controller, and only keep the SDC/block specific parts in the source plugin. Adding a @todo to cover this.

e2e is finally passing, but only via adding an arbitrary cy.wait() before clicking the Admin menu block in the the layout to open its block settings form. Without this wait, clicking on the block does nothing. Clearly there's a process that needs to complete before clicking the block, but it's not yet clear what that is. Will keep digging.

Yikes that was a long time spent on one tiny e2e test.

I tracked this down to the ComponentOverlay component that sit on top layout nodes in the iframe that receives the click to trigger opening of its edit form. When its a block in the layout, the corresponding ComponentOverlay briefly lacks the correct click handlers to. It then re-renders and it works. The longest it has taken to rerender in my test runs is 28ms,

I added a line to e2e test that effectively waits for this rerender

   cy.get('#cea4c5b3-7921-4c6f-b388-da921bd1496d-name').should((blockName) => {
      expect(blockName).to.have.text('Administration');
    });

During the initial not-working render, the text of #cea4c5b3-7921-4c6f-b388-da921bd1496d-name is just component. Once it's Administration it means the click handlers are working too.

Obviously we shouldn't have component overlays that don't work until a 2nd render, but I'm not 100% sure this is something caused by any of the changes here - this are of the FE wasn't really touched. If there's no evidence the changes here are introducing the problem I'd recommend landing this so we can progress with block integration and the broken-for-a-few-ms ComponentOverlay can be tackled in another issue.

Merged in 0.x, unassigning so someone else can review.

👏 @bnjmnm and @longwave for splitting this up in 2 parts in #14.

[…] it is not clear how to refactor this until we also have block settings values arriving from the front end, then we can figure out commonalities and move as much as possible into the controller, and only keep the SDC/block specific parts in the source plugin. Adding a @todo to cover this.

Makes sense! This MR contains a significant iteration, and that's all that matters!

@bnjmnm in #16: woah, that sounds like an epic but frustrating bit of research! 😳

Reviewed! Looks great! Basically only clarification requests. I approved this MR by assuming that the necessary clarifications happen prior to merging. Please clarify, then merge — great work! 😄

wim leers → credited larowlan → .

I suspect we'll need to resolve this too, the @todo points here

— @larowlan at https://git.drupalcode.org/project/experience_builder/-/merge_requests/3..., commenting on BlockComponent::hydrateComponent()

Lee is right.

BUT!

#2 is not true anymore, because 📌 Add setting to enable XB for page templates Active was forced to land a low-level piece of this: it needed \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::hydrateComponent() to handle block plugin settings and
— see https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

Still, we need this MR to be rebased and ensured that that is working as intended with all changes of this MR.

Shoot, this MR was green and seemingly ready for (re-)review since Friday night! Will get on it.

This also conflicts with 📌 Move form state into the global store Active , which landed on Saturday. The conflict is simple enough to resolve, but IDK yet if tests will pass. Let's find out! 🤠

Be back after lunch…

I can locally reproduce the cypress E2E test failures:

      cons:error ✘  TypeError: Cannot read properties of undefined (reading 'values')

I'm sure that takes somebody actively working on the UI an order of magnitude less time to fix than it does for me 😅

I already approved this MR 4 days ago in #20 — that's still true: I'd love to see this land sooner rather than later! 😄

Looking into fails

Overall this looks great, so I merged it to 0.x. @wim leers: can you open a new issue (or issues) for your nits and clarification questions that didn't get resolved/answered? Thanks!

This unblocked 📌 Implement saving block settings forms Active !