Enhance instances list for better navigation

The `entity override instance` should only display the list of instances corresponding to different languages ​​for the current entity when the current entity is being overridden. This list is essentially a translation list. Otherwise, it should not be displayed by default, and the number of instances could be very large.

translation override issue here need review:
https://www.drupal.org/project/display_builder/issues/3611826

@Jean Excellent planning. I hope the translation issues can be resolved soon. I'm preparing to use DS in the production environment.

I take time reviewing this MR because, if merged in the current state, it will count for 5% of the PHP code (comment, blank lines & tests excluded) of the full project.

Code overview

Most of the added logic is in InstancePanel and in the implementations of those 3 new DisplayBuildableInterface methods:

• ::getDisplayLabel(): ?string;
• ::collectDisplays(array $options = []): array;
• ::collectDisplaysBound(): ?TranslatableMarkup;

Code review

⚠️ InstanceInterface::getBuildable(): ?DisplayBuildableInterface; has been added with the corresponding implementation but it is called only in:

• in Instance::label() so inside the same class
• in the corresponding InstanceTest::testLabelWithoutBuildable()

⚠️ Why DisplayBuildablePluginBase::getViewModeLabel() and ::getBundleLabel()? This logic (and the corresponding entity_type.bundle.info) seems specific to display_builder_entity_view scope, do we?

⚠️ ::getPrefix() has been removed from the DisplayBuildableInterface but it is still a public method in DisplayBuildablePluginBase. Why? For information, in #3573905: Simplify DisplayBuildableInterface we successfully moved this method to protected.

⚠️ A new "floating" DisplayReference utility class (called by InstancePanel and implementations of DisplayBuildableInterface::collectDisplays()). There is nothing in the Drupal API which can "carry" this logic? If not, it is OK.

⚠️ ContextualFormPanel is the second most changed file of the MR but it is hard to see how it belongs to the current work. Some added logic is specific to MainPageContentSource from display_builder_page_layout according to the comment but to display_builder_entity_view according to the code.

⚠️ I have a warning in every display builder page, after switching to the branch, a updb and cache rebuild:

Undefined array key "instance_id" in Drupal\display_builder_test\Plugin\display_builder\Buildable\TestDisplayBuildablePlugin->__construct() (line 53 of TestDisplayBuildablePlugin.php).

Feature review

Still exploring, looks cool so far... but we can discuss the code review first.

By the way

In a #3542796: Preview of view sources comment, it was said:

A change of preview placeholder is proposed in #3615031: Enhance instances list for better navigation so probably better to have it merged before, then back to this one, then a follow up issue for views placeholder.

But i didn't see this while doing the review.

DisplayReference kept, and I would argue it is a value object rather than a floating utility.

OK, good, no issue with that. I was just asking if there was already something defined in the Drupal world we can leverage.

I will resume my review soon, with a focus on the functional part.

I still have this warning on display builder pages:

Undefined array key "instance_id" in Drupal\display_builder_test\Plugin\display_builder\Buildable\TestDisplayBuildablePlugin->__construct() (line 53 of TestDisplayBuildablePlugin.php).

Review in progress.

Instance sidebar

The feature is cool but it looks like a power users one which have a risk to confuse the casual users. We need to be especially careful because of those 2 reasons:

• it is the only island not telling about or active on the display we are currently building, but about a global state of the website
• the island is activated by default, so will be seen by most of the users

I am genuinely asking myself (no answer yet, not pushing anything for now):

• Do most of the users need all this information? Seeing all buildable types (Page Layout, ) instead of only the one we are working one? Seeing the "not built yet"? Direct access to the settings (which are rarely used once Display Builder is activated for a display)?
• Do we accept the added code load (still an additional 5% of PHP) for this feature? Is focusing the UX also a way of simplifying the code?

⚠️ The display we are currently building is also shown in the list. When i start a new display, i see it as "empty", it stays empty when i publish it. I need to refresh the page to see the "empty" removed.

⚠️ "Not built yet" means "not manage by display builder" and show displays which can be built from other tools.

⚠️ The settings link of "Entity View Override" is loading entity view display page. Is it the one people doing content override need? Would the entity edit form be better?

ContextualFormPanel

I am still struggling to understand the change: "Build the line naming the display this node renders, when there is one." for 3 reasons:

• It doesn't seem related to the rest of the MR and may deserve its own discussion and work
• It is still very bound to entity_view with a logic around entity types, bundles, view modes, targets...
• What do i need to do to test it in the UI? I am building an article display, using an entity reference field to tags vocabulary and a rendered entity formatter to a display also build with Display Builder, I see nothing special in the panel.

cut the ContextualFormPanel change.

Great, but we have still the corresponding line in .phpmd.baseline.xml:
<violation rule="PHPMD\Rule\Design\WeightedMethodCount" file="/builds/project/display_builder/src/Plugin/display_builder/Island/ContextualFormPanel.php"/>

Instances UI

A lot of markup and CSS has been generated for this feature, especially in /assets/css/instances.css. Because of its specific design implementation, the panel looks weird, a bit out of place in the Display Builder UI, with the bullet list, specific sizing and spacing, underline blue links..:

We are using a design system, Shoelace, to keep UI consistency and to delegate design choices and implementation to upstream. So, why not using Shoelace components here? For example:

• sl-menu-label instead of .db-instances__title
• sl-menu-item instead of .db-instances__item, the settings link can be put in suffix slot
• sl-menu-item[disabled] instead of .db-instances__item--not-built
• sl-divider in between groups
• ...

Also:

• Every "not built" list item as the popover title: "Not using Display Builder. Enable it on Manage display.", whcih make sense only for EntityView buildables.
• "Filter" label in displays filter bar would be more explicit as "Filter display"

About Views

There are 2 issues:

• "not built" list items are displayed as an empty list item instead of hidden (see screenshot)
• displays not buildable with Display Builder are present in the list. With a fresh Drupal 11.4 install, more than half of the listed displays don't belong here.

Because I have already play with a solution while investigating, I will propose a little commit (in a different MR of course), so i keep the ticket on my side for a few minutes more.

By the way

As previously stated, it was said on a #3542796: Preview of view sources comment:

A change of preview placeholder is proposed in #3615031: Enhance instances list for better navigation so probably better to have it merged before, then back to this one, then a follow up issue for views placeholder.

Is it work expected in another ticket?

Commit pushed (in a distinct MR to not mess with your work): https://git.drupalcode.org/project/display_builder/-/merge_requests/346/...

Addressed:

• ✅ "Filter" label in displays filter bar would be more explicit as "Filter display"
• ✅ Views display: "not built" list items are displayed as an empty list item instead of hidden (see screenshot)
• ✅ Views display: not buildable with Display Builder are present in the list. With a fresh Drupal 11.4 install, more than half of the listed displays don't belong here.

I can also try to propose the conversion of instances.css to Shoelace if you want.

For placeholder as I noted on comment #16 that this job has been removed from this issue as it was making it too big. We'll require an other issue which I have locally but not created yet.

OK thanks, i missed this. Let's wait the next issue.

So let move to button component, I do not think menu is valid here as I want to match neighbor 'Libraries'.

Cool.

So, we reached RTBC, with only one last change before merging: are you OK to add 2 more conditions for the list of View displays?

--- a/modules/display_builder_views/src/Plugin/display_builder/Buildable/ViewDisplay.php
+++ b/modules/display_builder_views/src/Plugin/display_builder/Buildable/ViewDisplay.php
@@ -396,6 +396,14 @@ final class ViewDisplay extends DisplayBuildablePluginBase {
       return FALSE;
     }
 
+    if (\str_starts_with($display->getOption('path') ?? '', 'admin/')) {
+      return FALSE;
+    }
+
+    if ($display->getOption('use_admin_theme')) {
+      return FALSE;
+    }
+
     return TRUE;
   }

They are inspired from core/modules/views/src/Plugin/views/display/Page.php, lines 260-265, and they skip displays which are rendered with admin theme: /admin/content, /admin/people...

However, do we need to consider the View displays rendered with admin theme but where Display Builder has been used?

Edit: Oops, I am not sure we have addressed everything from #3614998: Island instances list navigation enhancement:

• is "Instances" the correct wording here? It is the technical term and the one we use in Admin UI, for site admins, but it may confusing for casual users. Why not something like "Displays", "All displays" or "Displays overview"?
• do we display the instances of the same "buildable" first instead? For example, if we build a page layout, we have the page layouts list first?