Problem/Motivation

This issue is not related to #3529095: Adopt navigation module's top bar but leverage the same Core APIs.

Users don't easily know if a display has been built with Display Builder or not. This is particularly confusing for Page Layouts, because they obey indirect and sometimes complex visibility condition rules.

Proposed resolution

Mockup:
mockup

Instead of an ugly link, it would be better to use an icon, with or without a label. Or the logo, once #3544785: New logo.png for the project is resumed?

Proposal 1: only page layouts

When a page is built with Display Builder, add a toolbar item. This one is straightforward.

Proposal 2: page layouts & main content

When a page, or the main content of a page, is built with Display Builder, add a toolbar item with a link.

We will have 2 links or twice the same icon. How do we manage this?

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

Assigned: Unassigned » pdureau
Category: Task » Feature request

let's go :)

pdureau’s picture

Work in progress:

  1. "Page layouts" plugin: DONE
  2. "Main content" plugin: TODO

pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Active » Needs review

Finally, I prefer do Page Layout only because for other buildable in main content (entity view with full display, views with page display), there may be better tools like the little menu directly wrapping the content (i don't remember the name), or local tasks plugins to be listed in \Drupal\navigation\Plugin\TopBarItem\PageActions

mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Needs review » Needs work
StatusFileSize
new55.16 KB

The icon will appear on the display builder itself, which makes no sense with an empty top bar and an offset problem.

top bar page layout

Note that to get current display label is purpose of getDisplayLabel in #3615031: Enhance instances list for better navigation. Probably to have first and then we can decide where to use it.

From current state this work seems to makes sense only when we migrate fully to Navigation top bar? Or we must isolate to when 'view' a display and not edit.

pdureau’s picture

Assigned: Unassigned » mogtofu33

The icon will appear on the display builder itself, which makes no sense with an empty top bar and an offset problem.

Oh! thanks. I didn't check this. I will add a condition to not print this for display builder pages.

Note that to get current display label is purpose of getDisplayLabel in #3615031: Enhance instances list for better navigation. Probably to have first and then we can decide where to use it.

We can wait but I am not sure we will use ::getDisplayLabel() here. Telling than the page is build with Display Builder may be enough. De we need to tell there which page layout config entity has been used?

From current state this work seems to makes sense only when we migrate fully to Navigation top bar? Or we must isolate to when 'view' a display and not edit.

This is an optional feature for people using Core's Navigation module. This is not related to #3529095: Adopt navigation module's top bar

pdureau’s picture

Assigned: mogtofu33 » pdureau

oops, i wanted to assign to me and i misclicked :)

pdureau’s picture

The icon will appear on the display builder itself, which makes no sense with an empty top bar and an offset problem.

Fixed, but still some cache issues. Keep working on it.

pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs work » Needs review
  • Route conditions: Added
  • Cache issues: Fixed
  • Pipeline: Green

The message has been moved to the end of the toolbar (TopBarRegion::Actions) because it is where this seems to be expected (if i put it in start or middle region, the toolbar is always visible, even when empty).

DisplayBuildableInterface::getDisplayLabel() is now available, but I preferred to not use it, to keep the message short. Whatdo you think?

mogtofu33’s picture

Assigned: mogtofu33 » pdureau
Status: Needs review » Needs work
StatusFileSize
new190.74 KB

We try to provide user with 1 information and 1 related action and we live side of existing possible content edition in toolbar. And I noted behavior of title is different when sidebar is open and we have the small viewport cases as well.

Here is the result, left is an editable content, right is a regular page that now have the top bar because of this link.

sample

Mix Text and icon that will swap with left sidebar behavior seems strange and not good.

For left case when living with edit we should probably be in the 3 dots menu as 'Manage display' only.

For right case it's annoying because we trigger an empty top bar, so I think it is too soon for us to plug there.

pdureau’s picture

For left case when living with edit we should probably be in the 3 dots menu as 'Manage display' only.

To achieve that, we need to add a local task plugin corresponding to the roue, because this menu is managed by \Drupal\navigation\Plugin\TopBarItem\PageActions plugin which is collecting the local tasks.

It is doable using a deriver, I guess, but:

  • the link may show us at other places when local tasks are collected
  • I don't know how to manage cache with local task plugins. It seems the link is here or not here, and any change in PageLayout entity will not be propagated without a full cache rebuild
  • the UX discovery is not great

Mix Text and icon that will swap with left sidebar behavior seems strange and not good.

Maybe a button would be better. I will try that, and I will shorten the label: from "Page built with Display Builder" to "Edit page display"

For right case it's annoying because we trigger an empty top bar, so I think it is too soon for us to plug there.

I don't see the issue us being the only interactive element of the toolbar on some pages. Maybe it will be less weird with the button. Let's try.

pdureau’s picture

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

Still a low priority feature, but a nice one :)

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

mogtofu33’s picture

pdureau’s picture

Rebased

mogtofu33’s picture

mogtofu33’s picture

Assigned: mogtofu33 » pdureau

I think it would be better to have a global vision of navigation top bar integration as for me we bring confusing asynchrone links behaviors between our builders.

My proposition is global #3624760: Plan: consistent Display Builder entry points in the Navigation top bar to have the goal and discussion with child issues for each.

A proposal for the implementation here, so the other entries can build on it:

  • Instead of a featured button, a text-only "Display" item in the actions region, weighted after core's page actions, opening a dropdown. Core's "Edit" stays the only featured action, so we no longer have two primary buttons with ours first.
  • The item lives in the main module; each submodule adds its entry. This issue adds the first one: the page layout.
  • The entry says what it edits and how far a change reaches, e.g. "Page layout: Articles", with "Pages matching its conditions", rather than "Edit page display", which reads as "edit this page".

Main content and Views entries would follow as separate children of the plan, reusing the same item.
The top bar inside builders is out of scope here: the plan proposes reverting #3543335: Navigation top bar not present on entity overrides so no builder shows it until #3529095: Adopt navigation module's top bar.

What do you think? MR !344 already does most of the work (route checks, cache tags, access); it is mostly the rendering and the label that change.

pdureau’s picture

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

A proposal for the implementation here, so the other entries can build on it:

I love the idea.

What do you think? MR !344 already does most of the work (route checks, cache tags, access); it is mostly the rendering and the label that change.

Great. So, how do we process? We merge this ticket first (to get the "deep" logc) and continue the work/discussion in #3624760: Plan: consistent Display Builder entry points in the Navigation top bar?

The top bar inside builders is out of scope here

I agree, this is totally unrelated to #3529095: Adopt navigation module's top bar.

The plan proposes reverting #3543335: Navigation top bar not present on entity overrides

I don't see the relation here, but let's revert if if needed. It would be nicer to have the revert as a commit inside a MR instead of in the main branch, but this is only an esthetic preference, no big deal.

mogtofu33’s picture

Will merge, for the revert #3543335: Navigation top bar not present on entity overrides: it is the only reason one of our four builders shows the core top bar. Its link templates make Navigation treat /node/{node}/display/{mode} as a content entity route, so the override builder gets the core top bar stacked on top of our own toolbar, while the page layout, bundle display and Views builders get none.
The plan picks "no top bar in any builder" as the consistent rule for a fresh start.

And yes, the revert will be a commit in an MR, not straight on the main branch, hence the child issue #3624761: Remove the Navigation top bar from the entity override builder

  • mogtofu33 committed 41328afb on 1.0.x authored by pdureau
    feat: #3616421 Add a navigation toolbar item in page built with Display...
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.

Status: Fixed » Closed (fixed)

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