Problem/Motivation
Some current and planned tasks may be related:
- BuilderPanel is too complex (see #3559572: Evaluate and refactor buildSingleBlock), and TreePanel & LayersPanel are too tightly coupled to BuildePanel. So maybe some logic must be moved elsehwere.
- Some proposals for slot management (for example: #3569389: Improve UX for adding content to empty slots and #3538730: UI parent selection. Maybe some parts of #3544026: Cardinality & suggested components constraints for slots and #3551232: Add a lock system) will be better implemented by a dedicated island plugin
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:
- Add a new slot handler island typeIn IslandType, in ProfileForm...
- 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?
| Comment | File | Size | Author |
|---|
Issue fork display_builder-3594176
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
Comment #2
pdureau commentedComment #4
pdureau commentedLet's start
Comment #5
pdureau commentedExciting progress.
I believe the MR is now a suitable proposal, with the expected:
BuilderPanel::build(),BuilderPanel::buildComponentSlot()andLayersPanel::buildSingleComponent()logic to the newDropzoneislandAnd also:
SlottablePanelInterfaceforBuilderPanel&LayersPanelNeeds work:
Comment #6
pdureau commentedI 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.
Comment #11
mogtofu33 commentedHad 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.
Comment #12
pdureau commentedThanks for the extensive review. I will have a look.
Comment #13
mogtofu33 commentedThe API errors seems not related, I will have a look with #3608901: Component patterns must be wrap to work in the builder
Comment #14
mogtofu33 commentedI confirm, the error is on the "pattern" component not wrapped, here we only have the config issue.
Comment #15
pdureau commentedOk Great news
Comment #16
pdureau commentedBranch rebased (ans squashed, to make the rebase easier).
Indeed, if i create a new profile while in branch 2.0.x, and then i switch to
3594176-slot-handler_rebasedbranch, the profile don't get the island and is made unusable without an action.ddev drush entity:save display_builder_profileis 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_defaultmechanism is currently only used ProfileForm? Do we move it to Profile entity?Comment #17
pdureau commentedHi 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.