Show configuration forms for 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: the Term entity ID would be passed into a block setting, e.g. for a view blocks' contextual filter argument? If so, is that not already supported today … through contexts?

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. → #3485502: [needs design] Handle block contexts (aka implicit inputs vs `settings` aka explicit inputs))

#3484671: Add support for block derivatives 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: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc..

FYI, as I was refactoring the Accordion component in #3487484: Save page data form values in application state with support for undo/redo, 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 render 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 #3485502: [needs design] Handle block contexts (aka implicit inputs vs `settings` aka explicit inputs), the implicit inputs of block plugins is out of scope for 0.2. This issue/MR is handling the explicit inputs of block plugins. 👍

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 sits on top layout nodes in the iframe. The one that receives the click to trigger opening of edit forms. I was running into an issue where, the corresponding ComponentOverlay briefly lacked the correct click handlers. 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. It doesn't seem to happen with SDCs buuut this area of the FE wasn't really touched and it might be an underlying inefficiently vs a newly introduced proplem. If there's limited 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. The test workaround isn't *too* hacky, and the problem corrects itself faster than any human could perform the gestures needed to trigger it.

👏 @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! 😄

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 #3492669: Use PageTemplate status flag to enable per-theme page templates 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 #3492511: Move form state into the global store, 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 #3491978: Implement saving block settings forms!