Problem/Motivation

When we did #3562060: Render UI with admin theme , it was decided to not hide the "Default Admin" theme toolbar on display builder pages because there was uncertainties about how this toolbar is added and we didn't want to risk side effects by using a loosely defined hook.

I guess it was the good approach at this time, but we have now something kind of useless which is taking as much screen estate as our toolbar:

Proposed resolution

2 weeks after the switch to admin theme, we can try again as an isolated task.

Test with Claro, Gin contrib and the new default admin theme.

Let's keep the "do we need still need the Expand button?" question out of this discussion. It is tempting to ask such question once the Gin toolbar is removed. But it was discussed many times since July and it was confirmed as absolutely needed, even before this Gin toolbar was visible, even if I personally still struggle to understand this.

CommentFileSizeAuthor
#13 09-claro-nav-final-mobile-builder-entity-override-sidebar-expanded.png25.86 KBandres alvarez
#13 01-claro-nav-final-desktop-builder-entity-override.png67.87 KBandres alvarez
#13 21-claro-nav-base-desktop-builder-entity-override.png72.6 KBandres alvarez
#10 3633034 after + GIN.png325.23 KBcsakiistvan
#10 3633034 after.png376.67 KBcsakiistvan
#10 3633034 before.png300.11 KBcsakiistvan
estate.png22.05 KBpdureau

Issue fork display_builder-3633034

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:

About issue forks

Comments

pdureau created an issue. See original summary.

pdureau opened merge request !514

pdureau’s picture

pdureau commented
Status: Active » Needs review

default_admin has no top bar of its own: it re-themes the one core Navigation builds. So we need to address

MR ready for review: https://git.drupalcode.org/project/display_builder/-/merge_reque

I was not able to just copy what we already got in #3562060: Render UI with admin theme because it triggers a PHPMD NPathComplexity errror.

All tests with Drupal 11.4 and the navigation nodule installed.

Default Admin theme Claro theme
Default admin toolbar not visible in Display Builder page ✅ ✅
Default admin toolbar still visible in /admin/structure/page-layout with the expected breadcrumb and local actions ✅ ✅
Navigation toolbar (which is another toolbar) still visible in /user/1 with the expected title and buttons ✅ ✅

I keep on my side because of a phpunit functional error on OverrideDisplayTabsTest:

public function testCurrentDisplayIsExcludedFromItsOwnList(): void {
  ...
  $this->drupalGet(\sprintf('/entity_test/%d/display/full', $entity->id()));
  $this->assertSession()->linkExists('Display: Teaser');
}

Checked with /node/2/display/default which is the node entity equivalent of the test. It is weird because this links doesn't exist in those pages, with or without the current change.

pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned

I keep on my side because of a phpunit functional error on OverrideDisplayTabsTest.

I have added a check related to current theme:

private function isTopBarTheme(): bool {
   $theme = $this->themeManager->getActiveTheme();
   return $theme->getName() === self::TOP_BAR_THEME
     || \array_key_exists(self::TOP_BAR_THEME, $theme->getBaseThemeExtensions());
}

All tests with Drupal 11.4 and the navigation nodule installed:

Default Admin theme Claro theme
Default admin toolbar not visible in Display Builder page ✅ ✅
Default admin toolbar still visible in /admin/structure/page-layout with the expected breadcrumb and local actions ✅ ✅
Navigation toolbar (which is another toolbar) still visible in /user/1 with the expected title and buttons ✅ ✅
smovs’s picture

smovs commented

Hi team!

Tested MR !514 on Drupal 11.4.11 with Navigation, UI Skins 1.2.1 and Gin 5.0.16.

Page Default Admin Claro Gin
Display Builder page ✅ top bar hidden ✅ no top bar ❌ top bar still shown
/admin/structure/page-layout ✅ top bar shown ✅ unchanged ✅ top bar shown
/user/1 ✅ top bar shown ✅ top bar shown ✅ top bar shown

Gin's top-bar--gin.html.twig is the same as Default Admin's top bar, but isTopBarTheme() only checks default_admin. Making it also match gin hides the top bar on the builder page under Gin and keeps it on the other pages.

The MR's new test (ThemeHooksChromeTest) passes. OverrideDisplayTabsTest, reported as failing in #3, passes for me with and without the MR, so the MR doesn't break it.

pdureau’s picture

pdureau commented

Thanks Sviatoslav for testing with Gin. It was out of the scope of my review, but it is relevant. Can you propose a fix?

smovs’s picture

smovs commented

Instead of adding Gin to the theme check, I'd suggest basing the decision on the top bar's template rather than on the theme's name.

Core Navigation's own top bar renders nothing on builder pages, which is why Claro shows nothing there. The bar only gets in the way when something replaces its template with one that always renders and repeats the breadcrumbs, tabs and actions the builder toolbar already shows. Default Admin and Gin do this with a theme suggestion. A module can do it too: Gin Toolbar overrides the template, so even Claro shows Gin's full top bar.

So the rule could be: on builder pages, hide the top bar only when its template has been replaced by a theme or a module. The active theme's registry tells us that, so there's no list of themes to maintain. Unknown admin themes and modules are handled automatically. Core's own top bar stays, as intended in #4, the navigation sidebar is always kept, and other pages are unaffected.

Other options I considered:

  • A list of theme names: too narrow. Every new admin theme needs an update, and it can't catch a module that overrides the template.
  • Always hiding the top bar on builder pages: simple, but it drops core's top bar under every theme, which #4 meant to keep.
  • Putting the builder's items into Navigation's top bar: it fits how Navigation is designed, but it's a much bigger change and better as a separate issue.
  • A setting to hide the top bar: explicit, but a lot of extra config for something most sites wouldn't change.
  • CSS only: cheap, but fragile. It depends on each theme's class names and can leave an empty gap at the top of the page.

One trade-off: the replaced top bars also hold the mobile "Expand sidebar" button, so it would be gone from builder pages. The MR already accepts this for Default Admin.

If this direction sounds good, I can update the MR.

pdureau’s picture

pdureau commented

A list of theme names: too narrow. Every new admin theme needs an update, and it can't catch a module that overrides the template.

🤔 I am hesitating:

  • In a way, I agree adding Gin in the list is opening the Pandora box, and we will ahve to maitain a list of contrib themes.
  • In an other way, there is a movement in Core to make the admin theme switch more difficult (no UI, some hardcoded values), so this issue will be mitigated

Always hiding the top bar on builder pages: simple, but it drops core's top bar under every theme, which #4 meant to keep.

🤔 That's why we want to do in the scope of this ticket. Maybe the proposal of comment #3 was already the good one, and it would be better to fix the test than add a condition about theme, like what has been done in #4.

Putting the builder's items into Navigation's top bar: it fits how Navigation is designed, but it's a much bigger change and better as a separate issue.

❌ Already a separated issue: #3529095: Adopt navigation module's top bar, which has been postponed partially because Navigation Toolbar was initially designed to be displayed in front theme pages only. However, because Default Admin (and Gin) is now using it for admin pages, it may be relevant again.

It will not be easier though, because instead of hiding the toolbar on Display Builder pages, we will need to remove the ToolbarItem plugins which are not ours.

A setting to hide the top bar: explicit, but a lot of extra config for something most sites wouldn't change.

❌ No settings. Just sensible default behaviour.

CSS only: cheap, but fragile. It depends on each theme's class names and can leave an empty gap at the top of the page.

❌ Too fragile indeed.

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: Unassigned » csakiistvan
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Status: Needs review » Needs work
StatusFileSize
new 3633034 before.png300.11 KB
new 3633034 after.png376.67 KB
new 3633034 after + GIN.png325.23 KB

❌ Does not fix the issue — applied MR !514 on top of current 1.0.x with Navigation enabled, rebuilt caches and opened a page layout builder: under Default Admin the top bar is gone from the builder page and still shown on /admin/structure/page-layout, but with Gin as admin theme the top bar is still rendered above the builder toolbar. Gin, listed in the proposed resolution, is not covered yet (same as #7).

Prerequisites
  • Enable navigation, install default_admin; switch the admin theme with drush config:set system.theme admin gin.
  • Create a page layout and open /admin/structure/page-layout/{id}/builder.
Code review
  • MR !514 at 7a4353a5, rebased on 1.0.x, pipeline green.
  • isTopBarTheme() only matches default_admin and its sub-themes; Gin (base theme Claro) ships its own top bar template, so it never matches.
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: csakiistvan » Unassigned
Status: Needs work » Needs review

Went with the approach from #3: the top bar is now removed on every builder page, whatever the admin theme. No theme list to maintain, and the behaviour is the same everywhere.

Side effect: the "Display: …" links from core Navigation's top bar are gone from entity view builder pages. Restoring them belongs to #3529095: Adopt navigation module's top bar.

Tests are updated and pass. Please test with Navigation enabled under Default Admin, Claro and Gin: the builder page should have no top bar, while /admin/structure/page-layout and /user/1 keep it.

smovs’s picture

smovs commented
Status: Needs review » Reviewed & tested by the community

Hi @csakiistvan! Thanks for the update.

Tested the latest commit of MR !514 (148bdae) on Drupal 11.4.11 with Navigation, Gin 5.0.16 and Gin Toolbar 3.0.3.

Page Default Admin Claro Gin
Display Builder page ✅ top bar hidden ✅ top bar hidden ✅ top bar hidden
/admin/structure/page-layout ✅ top bar shown ✅ top bar shown ✅ top bar shown
/admin/structure/display-builder/preset ✅ top bar shown ✅ top bar shown ✅ top bar shown
/user/1 ✅ top bar shown ✅ top bar shown ✅ top bar shown

The navigation sidebar is kept on every page.

For comparison, on the previous commit (7a4353a) the top bar was still shown on the builder page under Claro and Gin. Claro shows it there because Gin Toolbar overrides the top bar template, as mentioned in #7. With 148bdae it is gone under all three themes.

The trade-off I raised in #7 doesn't apply: the mobile "Expand sidebar" button is still on the builder page under Default Admin and Gin. It lives in Navigation's control bar, not in the top bar.

ThemeHooksChromeTest and OverrideDisplayTabsTest pass locally.

Losing the "Display: …" links on entity view builder pages is expected and covered by the updated test. Restoring them belongs to #3529095: Adopt navigation module's top bar.

Moving to RTBC.

andres alvarez’s picture

andres alvarez commented
StatusFileSize
new 21-claro-nav-base-desktop-builder-entity-override.png72.6 KB
new 01-claro-nav-final-desktop-builder-entity-override.png67.87 KB
new 09-claro-nav-final-mobile-builder-entity-override-sidebar-expanded.png25.86 KB

I tested commit 148bdae of MR !514 on cases the previous comments did not cover: Claro as the admin theme, mobile, and a site without Navigation. All on Drupal 11.4.6 with Firefox, on throwaway test sites. Before and after compare against 1.0.x (654df5a).

  • Claro + Navigation, entity override builder (/node/1/display/full): with the MR the top bar is gone and the sidebar stays. On 1.0.x the bar showed (title, Published, Edit and the Display menu). Screenshots 1 and 2.
  • Page layout and preset builders, and their listings: no top bar, with or without the MR. Under Claro, Navigation's top bar is only drawn on non-admin paths, so the MR changes nothing visible on those routes.
  • /user/1 and /node/1: the bar is kept, as expected.
  • Mobile (390x844): the "Expand sidebar" button is still there and opens the sidebar, same as 1.0.x. Screenshot 3.
  • Without Navigation (core's classic Toolbar): the toolbar is intact on the builders, with no difference between 1.0.x and the MR. The admin menu is not lost.
  • Tests: ThemeHooksChromeTest and OverrideDisplayTabsTest pass locally.

One observation, not blocking the RTBC: under Claro, on the entity override builder the Edit button, the ⋮ menu and the "Display" dropdown also disappear, so the link back to the entity is gone. I understand that is the intended effect and that restoring it depends on #3529095.

Not tested: Gin, Default Admin, Drupal 11.4.11 or other browsers (already covered by @smovs in #12).

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

mogtofu33’s picture

mogtofu33 commented
Status: Reviewed & tested by the community » Fixed

Thanks for the work here.

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 fa465646 on 1.0.x authored by pdureau 
    task: #3633034 Hide default admin theme toolbar

By: pdureau
By: smovs...