Problem/Motivation

When I configure a Display Builder profile,
I want the View panels to be where they were designed to be,
so I can stop choosing between placements that were never meant to work.

For View panels the choice is not real, libraries or navigator makes no sense in the main, as well as builder on sidebar. Historically preview could make sense but is now shipped separately as a split view in main.

The flexibility of regions create permutations nobody test.

The precedent already exists in the codebase: Floating islands declare attach_to as a fixed plugin property, deliberately not admin configurable, for the same reason.

Steps to reproduce

Proposed resolution

Make View panel placement structural, owned by the plugin.

  1. IslandType::regions() — remove the View case. ProfileForm already
    handles an empty regions() result, so the Region column stops rendering for
    View islands with no form change.
  2. ProfileViewBuilder::prepareViewIslands() — read the plugin definition's
    default_region instead of $configuration['region' for the sidebar/main
    split.
  3. Profile::setIslandConfiguration() — the region preservation branch stays,
    Button islands still use it. Fix the comment.
  4. config/optional/display_builder.profile.default.yml — drop the now dead
    region: key from the View islands.
  5. hook_post_update — strip region from View islands in stored profiles, so
    exports do not carry a key nothing reads.

Out of scope, on purpose:

  • Button region stays configurable. save_status overrides the plugin
    default to sit at the toolbar start, and left or right of the toolbar is a
    genuine preference with no wrong answer. The schema key and the last
    IslandType::regions() case stay for it. Whether toolbar placement should
    also be structural is a product decision, not this cleanup.
  • The library_flat, library_tabs_display, contextual_tabs_display and
    view_panels_display knobs. Separate issue.

User interface changes

The Region radio disappears from the profile form for View panels. It stays for toolbar buttons. No change to the builder itself.

API changes

`IslandType::regions()` returns an empty array for the `View` type. Any custom View island plugin relying on a profile stored region now needs `default_region` on its `#[Island]` attribute.

Data model changes

None to instance state. Profile config only: the `region` key on View islands becomes ignored, then removed by the post_update hook. The schema key stays, Button islands still use it.

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

mogtofu33 created an issue. See original summary.

mogtofu33’s picture

Issue summary: View changes
mogtofu33’s picture

mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Needs work » Needs review
mogtofu33’s picture

Assigned: Unassigned » pdureau
mogtofu33’s picture

Issue summary: View changes
mogtofu33’s picture

Issue tags: +AI-accelerated

mogtofu33 changed the visibility of the branch 1.0.x to hidden.

pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs review » Needs work
StatusFileSize
new79.32 KB

So, we have for the main area:

  • BuilderPanel
  • LogsPanel
  • ScaffoldPanel

For the sidebar:

  • InstancesPanel
  • LibrariesPanel
  • TreePanel

But they are all mixed in the Profile edit form:
screen

Which is confusing, because we drag and mix stuff which are always separated at the end. I know it was more or less the case already, but at least there was a reason for that.

Also, the default_region attribute has now 2 different meanings, as explained in the doc:

`default_region`: For `IslandType::View` islands, `sidebar` or `main`: where
the panel renders. This is structural, not a profile preference, so a View
island without it renders in the main area. For `IslandType::Button`
islands, `start` or `end`: the toolbar side the button defaults to, which a
profile can override.

Why not clarifying/simplifying this by splitting the IslandType::View between something like IslandType::Main and IslandType::Side?

mogtofu33’s picture

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

Why not clarifying/simplifying this by splitting the IslandType::View between something like IslandType::Main and IslandType::Side?

Right double usage of default_region could have a fix, but split seems problematic:

IslandType match a slot in render pipeline to components/display_builder/, the deferrable feature (raise +1 case), prepareViewIslands is impacted, and DOM class used for css/js. Main and Side will be extremely close codewise (base class, traits, event, keyboard,...).

So the collateral is important and a split will inflate instead of simplify, with a bunch of Playwright affected. I feel it require a more profound refactor to simplify, which for now seems to me not worth it.

AI recommend 2 new enums:

IslandRegion::main|sidebar and ToolbarSide::start|end to fix the mix of usage/impact/meaning between buttons and viewPanels. So no more default_region remove IslandType::regions(), and we add IslandInterface::getRegion() IslandInterface::getToolbarSide()

Benefit is no css/js Playwright impact, possible easier post_update, better clarity for Island builders.

Wdyt?

pdureau’s picture

Assigned: pdureau » mogtofu33
StatusFileSize
new124.05 KB

Right double usage of default_region could have a fix

Only this would be kind of fine, but this alongside the UX issue in admin UI is concerning. We need to find a way to fix both together.

So the collateral is important and a split will inflate instead of simplify, with a bunch of Playwright affected. I feel it require a more profound refactor to simplify, which for now seems to me not worth it.

OK. Let's try to simplify and not make a mess.

AI recommend 2 new enums: IslandRegion::main|sidebar and ToolbarSide::start|end to fix the mix of usage/impact/meaning between buttons and viewPanels. So no more default_region remove IslandType::regions(), and we add IslandInterface::getRegion() IslandInterface::getToolbarSide()

Sorry if I am rounding about the AI proposal, but I am still sorting this out.

So, we already have 2 mechanisms in play here: the IslandType and the region, and it would be great to not add more.

Can we keep those two with a single change: regions are not a configurable value anymore? Neither for view panel (as already stated by your MR), nor for buttons:

  • We have an unified mechanism again. I still like the idea users can distribute button between the start and the end of the toolbar in configuration, especially once they have a lot of button islands activated, but if this move is the change we need, let's go.
  • default_region attributes simply become region
  • We can keep the existing rendering logic about regions in ProfileViewBuilder
  • We don't necessary need to add 2 new enums, we can keep IslandType::regions() and put the View panels one back there (as it is currently in 1.0.x).

This will also allow us to address the UX issue: instead of a "region" table column in the admin UI, we print a distinct table by region in the panel type. Mockup for View Panels:
mockup

It would be easier for users to understand:

  • An island type can have many regions
  • I can reorder the islands of a same region, but i can't move an island from region to region

This change nothing in the storage, which is still a flat list of islands with weight:

   builder:
    status: true
    weight: -8
  scaffold:
    status: true
    weight: -6
  logs:
    status: false
    weight: -4
  preview:
    status: true
    weight: -5
  library:
    status: true
    weight: -10
  tree:
    status: true
    weight: -9

What do you think?

mogtofu33’s picture

Assigned: mogtofu33 » pdureau

default_region → region, non-configurable for buttons too. IslandType::regions() keeps both types, and the admin UI prints one table per region. Storage unchanged, post_update_3 now strips region from every island.

One catch: save_status was region: start in the shipped profiles while the plugin declared end. Fixed the plugin to start, so nothing moves. Every other island already matched its plugin default. We could follow up, and as discussed it could be a task with history/state/logs to decide where to bring the logs messages and mostly merge or deprecate save_status then.

Region is resolved once in IslandPluginManager::processDefinition(), so an invalid value falls back to the type default instead of vanishing from the form.

Unrelated but had to add a bit of cleanup to clear legacy, mostly to round up tests that this one touch.

pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs review » Reviewed & tested by the community

Ok, that's great.

mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • mogtofu33 committed 598d8fb6 on 1.0.x
    task: #3614990 Reduce ProfileViewBuilder permutations and have fixed...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.