Integrate Navigation with Workspaces

Added quick proto here.
I tried to reuse all existing components as much as possible.

I really think it shouldn't be `workspaces only` MR. We need to create some common UI for that panel. And probably for layout builder/announcements etc panels aswell.

So I've added .toolbar-panel in temporary styles.

@ckrina please take a look! Thanks

Adding link to related issue in Workspaces #3096017: Finalize the Workspaces UI.

@ckrina

#4: I agree we should also take a look around what things other core/contrib will want to add to this tool.
We need to keep in mind that other modules 'opt-in' to using toolbar via hook_toolbar:

• Workspaces: add a 'tab' (only) and attaches js/css via library api (which opens the top fixed off-canvus drawer that is the Workspaces UI)
• Acquia Connector: add a tab (only) opens to external hosting tools dashboard.
• Devel: adds 'tab' and 'tray' of child menu item [see: \Drupal\devel\ToolbarHandler]
• Env Indicatior: general use cases, add a'tab'
• demo_umami: adds 'tab' item to say This site is intended for demonstration purposes.

^
- IMO: umami is a good example of a bad use of hook_toolbar. overall message is dont' do what i do at that (:
- I would anticipate that devel would themselves convert to a menu and be placed in within the 'default/main navigation layout section' (left side). However this is reminding of the purpose of top bar: for these such of items!

Propose: Top bar also adds Layout for right/left position.
-> convert 'local tasks' to a plugin to be placed. q: Is NavigationBlock currently flexible enough to also be used this way
-> info and docs about the plugins.

DX:
->how can other modules install their items?
[edit] navigation.block_layout.yml shows structure.

Moving to Needs work after getting Navigation into core.

Actually, also postponing it until we have designs for this so nobody else tries to implement something that has to be changed later.

Following as this also impacts Tour (which is now contrib)

Navigation should have a 'pill' plugin item that lets contrib/core components simply add this 'link' for this wider 'task markers'
- contextual 'edit'
- workspaces
- HELP module
- umami 'demo notice link'
- Commerce activity
-admin toolbar 'search'

re: shortcuts #3445184: Fatal error when accessing Navigation Blocks on a minimal profile installation because of the Shortcut block

Came here as I was doing screenshots for Drupal 11's release and the navigation screenshot looks great, but I could only screenshot the stable workspaces module with the old toolbar. So we'll have these two screenshots in the Drupal 11 release.

Toolbar screenshot:

Workspaces screenshot:

Is there an issue for the design that this is postponed on?

Expand title and summary.

I've updated the first round of designs so the implementation of this can start. This will still miss hover states, for example. I've also added the new icon SVG.

I gorgot to mention that I opened some follow-ups for the rest of the modules to focus this issue only on Workspaces on the Navigation itself, so the rest of the modules have their own issues:

• #3465296: Integrate the Tour module into the Navigation
• #3465295: Integrate Top Bar Navigation with Contextual editing
• #3441100: Integrate Umami message
• #3465299: Integrate Announcements module into Navigation's drawer/submenu

Edit: For other ideas or integration pleas open issues for it so each issue keeps the scope contained.

This looks great. I think workspaces has three icons as shown in the screenshot (https://git.drupalcode.org/project/drupal/-/tree/11.x/core/modules/works... has the yellow and green page layout and the dimensional layers image), but the one in the toolbar is similar to the one you are proposing here, so that looks good too.

@Gábor Hojtsy yes, those other icons are used on the internal Workspaces UI we're still working on. :)

Thanks @ckrina for posting the designs, I also think they look good! My initial impression was that the workspace switcher is a "contextual operation", so the top bar would've been better suited for it, but I suppose there was enough thought put into the current proposal so I'm not going to argue about it :)

I've opened a MR with my initial attempt at this integration. No screenshot needed because it looks just like the existing ones.

This currently adds an alter hook, but otherwise the items in navigation are standard blocks. Should this be a block that is shipped with Workspaces instead?

@Gábor Hojtsy, the MR provides a Workspaces block as well. The hook is needed so modules can add blocks to the navigation without user intervention, basically what #3463142: Allow modules to hook into top of content section of new core navigation is also asking for.

Believe the new library has to be added to the Stable9 override list.

Discussed during meeting today.

Per @m4olivei:

It looks like this adds an alter hook that allows the form to be able to placed within the navigation module. We need to do two things:

• Does this solve our issue and allow us to place the form?
• Will this new hook work for other modules that want to do similar?

@m4olivei will work on this.

@mherchel, the purpose of the hook is described in #25..

I've added couple of comments in that MR

We need something like this
Styles and markup in Navigation and functional from Workspaces.
https://www.drupal.org/project/drupal/issues/3441100#comment-15770402

Just to confirm that this issue should only take care of the item placed on the Navigation itself. Nothing related to the form should be changed, since it really complicates the implementation and we need this issue fixed to get Navigation into stable. So as long we can trigger the old form from the new Navigation we can move any work to a followup (in Workspaces, and discussed & agreed with the Workspaces maintainers).

So, for this MR it'd be great if the color green used is closer to the one from the designs (the green chosen is darker and forces using white text).

1. we need to fix top displace
https://gyazo.com/668eaf04133ec59fd1c4026f89ef8e18

2. we need to fix use-ajax animation
https://gyazo.com/cfcc223672e9e4e5be6ecf24d0edc0ba

3. weird behaviour on mobile
https://gyazo.com/c8c93d79ba2ccdb6f9689907096263a9

4. it also has bigpipe replacement same as shortucts
https://gyazo.com/547da9f049c003ddfd371941b77046cd

To do it in good way, abstract buttons and css variables we will beed to update toolbar-button itself.
To avoid double work i would like to have https://www.drupal.org/project/drupal/issues/3458215 landed first.
It is also stable blocker.

Fixed most of the threads, there's only one remaining where we need a reply from @finnsky.

Points 1, 2 and 3 from #32 are still outstanding, but they're "general" problems with Navigation, not with the workspaces integration that we're trying to do here :)

Points 1, 2 and 3 from #32 are still outstanding

Yes. But scope of the current issue is navigation module. So i think we should fix it here aswell.

Also should this block be also configurable from UI?
Like
https://gyazo.com/71f405fd6ea357cc707056a5c3fc8bde

@crina could you please define animation icon
https://gyazo.com/86a2d286fe3d5bdb54d75e7619f6f107

I've fixed all issues from #32

But one important thing.

We have yet another issue. https://www.drupal.org/project/drupal/issues/3441100

Where we add messages into navigation via theme hook(s).
I would like to get backender review of both issues. to get best and simplest way to extend navigation.

Here for example we ignore that buttons(messages) can be displayed as list.

So before we will extend Navigation by other modules let's have universal extending mechanism

Ok, moved everything to the Navigation module.

Re #42: what kind of tests are needed?

I don't think that in this issue we should change core code. Let's create core issue about that parent class functions?

There's currently a single failing (Nightwatch) test left, and the problem seems to have been introduced in this commit https://git.drupalcode.org/project/drupal/-/merge_requests/9116/diffs?co.... No idea how to fix it, hoping that @finnsky can help :)

FYI the throbber doesn't work correctly when the navigation is collapsed on the front end theme.

Discussed with @plopesc and agreed that we should leave the "add the workspace navigation block by default" part to #3463142: Allow modules to hook into top of content section of new core navigation. I'm not doing that myself right now because I see @finnsky is actively working on the MR :)

We also discussed test coverage, and since the current MR is just adding a button that does the same AJAX action as the one from the Toolbar, there's nothing really testable at this point. Test coverage will be needed when we get to work on the full solution that will open the workspace switcher in the navigation popover.

Addressed the last feedback but this is sadly still blocked on a nightwatch fail that I've no idea how to fix.

Nightwatch fail says

   Timed out while waiting for element <.admin-toolbar__expand-button[aria-expanded=true]> to be present for 5000 milliseconds. - expected "visible" but got: "not found" (5157ms)

Test file can be found here: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/navig...

Thanks @plopesc for fixing the test! This is finally ready for review now :)

I would like to have it merged. It has some improvements about buttons and events.
On my side RTBC + 1

I would really like to have it merged as well, but I'm afraid we need to get #3463142: Allow modules to hook into top of content section of new core navigation in first.

Postponing this one on it to make it more explicit and have a better picture of the relationships between the issues.

Once we have #3463142: Allow modules to hook into top of content section of new core navigation merged, it will be only necessary to implement the hook to have the Workspaces block visible in the new Navigation.

Probably we will need to backport valuable CSS/JS changes from this MR then.
Currently we have bug with anything displaced from top.
Not only workspaces.

Also Ajax issues from #32 are global. So this prio maybe higher.

Updated the MR to use the new content_top region from #3491081: Allow other modules to include their own Navigation blocks during installation. Another artificial dependency down...

Code looks great and all the comments have been addressed.
Marking as RTBC.

Attaching screenshot to demonstrate the new behavior:

Thank you!

I apologize for chiming in at the end here. The interaction looks awkward to me. Specifically, there are 3 areas involved. The toolbar with the green Live, the expanding header where you begin the workspace selection. And then confirm dialog. This is a lof of jumping. Did we consider doing this all in one expanding menu, and doing away with the confirm dialog? Its not actually destructive to change your workspace. Its just your personal view of the content.

Please disregard this comment if the team disagrees,.

Did we consider doing this all in one expanding menu, and doing away with the confirm dialog?

Yup, that's the long-term plan per #30 :) While this issue is only focused on exposing Workspace's current switcher (with all its problems), in a followup we'll handle both suggestions. There's a screenshot in this comment #3457688-29: Support for core navigation module which shows the WIP design for the full Workspaces navigation UI.

What @amateescu said :)

All the early explorations we did trying to improve the workflow with Workspaces showed that we need to think about several existential interactions and UIs beyond the top dialog. That's why we kept this as "it only triggers the existing dialog" so we can get Navigation to Stable soon and not get into a way longer discussion that will block Navigation.

Looks "good enough" to me! Certainly better than now, which is no integration at all.

Do we actually have a follow-up issue open for the UX changes? Would be good to open that and maybe attach the most relevant screenshots from here. Tagging for that.

Opened one more follow-up based on the placeholder discussion in the MR: #3499871: Try to use a placeholder for the workspaces toolbar destination parameter.

Can't review the CSS here usefully, and only just about bits of the JavaScript, but other people have done that, and the PHP looks fine.

Committed/pushed to 11.x, thanks!

This may have potentially changed navigation performance tests.
After rebasing #3490841: Create hook_runtime_requirements() and hook_runtime_requirements_alter() I am getting failures for the navigation js performance test.

$this->assertLessThan(90000, $performance_data->getStylesheetBytes());

Added: #3500293: Add headroom to the navigation performance test