Problem/Motivation

Some current and planned tasks may be related:

Proposed resolution

1. Pass a full InstanceInterface between BuildePanel

Instead of string $instance_id as we already did in #3578999: Pass existing Instance object to islands methods instead of loading it again. This is necessary to be bale to send the current state (so, the instance entity) to the new island plugin.

This will also help consolidating the logic, because InstanceInterface is more specific than string. Also, this will remove the last remaining ambiguity between instance/node/builder terminology (following #3538360: Profile & Instance naming normalization.

We may extract this work to a dedicated ticket if it makes review simpler.

2. Slot handlers as island plugins

2 steps:

  1. Add a new slot handler island typeIn IslandType, in ProfileForm...
  2. Move BuilderPanel::buildComponentSlot() logic to its own island
#[Island(
  id: 'dropzone',
  enabled_by_default: TRUE,
  label: new TranslatableMarkup('Dropzone'),
  description: new TranslatableMarkup('...'),
  type: IslandType::Slot,
)]
class Dropzone extends IslandPluginBase {

Do we also introduce a mechanism which force at least one island to be activated for a specific type?

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

pdureau created an issue. See original summary.

pdureau’s picture

Issue summary: View changes

pdureau’s picture

Status: Active » Needs work

Let's start

pdureau’s picture

Exciting progress.

I believe the MR is now a suitable proposal, with the expected:

  1. Add a new slot handler island typeIn IslandType, in ProfileForm...
  2. Move BuilderPanel::build(), BuilderPanel::buildComponentSlot() and LayersPanel::buildSingleComponent() logic to the new Dropzone island

And also:

  • a new SlottablePanelInterface for BuilderPanel & LayersPanel

Needs work:

pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs work » Needs review

I moved some of the work to #3606874: Rename and document data-instance-id attribute, I rebased the MR, and the pipeline is now magically green.

So, this review must be done after #3606874: Rename and document data-instance-id attribute is merged.

mogtofu33 made their first commit to this issue’s fork.

mogtofu33 changed the visibility of the branch 3594176-slot-handlers-as to hidden.

mogtofu33’s picture

Assigned: mogtofu33 » pdureau
Status: Needs review » Needs work
StatusFileSize
new5.08 MB

Had some struggle with rebase, so created a other branch.

I have 2 strange issues:

If I had an existing profile config other than default, the display is empty, I guess we are missing an update for existing profiles to enable the slot handlers. Because if disabled, then the display is empty.
In the meantime we should probably have it without option to disable it in a profile?

An other strange issue is on 'builder' with a moderately complex data, like the default page layout on Bootstrap recipe, moving a component in a slot often bring API errors like (AttachToSlot], [MoveToSlot]... And looking at the log when have empty 'slot' and 'parent' values.

See video attached.

pdureau’s picture

Thanks for the extensive review. I will have a look.

mogtofu33’s picture

The API errors seems not related, I will have a look with #3608901: Component patterns must be wrap to work in the builder

mogtofu33’s picture

I confirm, the error is on the "pattern" component not wrapped, here we only have the config issue.

pdureau’s picture

Ok Great news

pdureau’s picture

Branch rebased (ans squashed, to make the rebase easier).

If I had an existing profile config other than default, the display is empty, I guess we are missing an update for existing profiles to enable the slot handlers. Because if disabled, then the display is empty.

Indeed, if i create a new profile while in branch 2.0.x, and then i switch to 3594176-slot-handler_rebased branch, the profile don't get the island and is made unusable without an action.

ddev drush entity:save display_builder_profile is not doing anything. But submiting the /admin/structure/display-builder/{profile} form is fixing it.

I am still hesitating about the way to address this:

  • enabled_by_default mechanism is currently only used ProfileForm? Do we move it to Profile entity?
  • do we add a new "default island" mechansim (if no island of a certain type is activated, we load this one)?
pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs work » Needs review

Hi Jean,

I have added a commit with a proposal: a new "default island" mechanism (if no island of a certain type is activated, we load this one).

I would be happy to discuss this with you. If we are both OK, we can decide what would be the hints in UI, so people will not be surprised if they uncheck everything and a slot handler is showing up anyway.