Problem/Motivation

Workspaces / navigation support is implemented, but we also need to move toolbar support so we can deprecate toolbar.

Steps to reproduce

Proposed resolution

Move toolbar integration from Workspaces to Toolbar.

Remaining tasks

Review.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

LLM disclosure

LLM was used to turn the removal into a move. With human review.

Issue fork drupal-3620812

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

catch created an issue. See original summary.

catch’s picture

Status: Active » Needs review

catch’s picture

This also removes workspaces support from toolbar. It would be theoretically possible to move all that logic across but really think people can move to navigation if they want all bells and whistles.

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

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now :)

gábor hojtsy’s picture

I was wondering the same by @catch above in #4 but I think its fine this way too.

This also removes workspaces support from toolbar. It would be theoretically possible to move all that logic across but really think people can move to navigation if they want all bells and whistles.

gábor hojtsy’s picture

I also looked at the scoping there was in #3618746: Toolbar uses in Announcement, User, Contextual originally, where this was broken out of. That was about moving the CSS along with the builders. This removes both so I think this is fine too. It would have been a problem if the CSS is removed but the builders were kept.

longwave’s picture

What if someone is using this functionality in 11.4 and upgrades to 11.5? Many users who started on 11.0 or earlier will still be using Toolbar and not Navigation, as it was experimental until 11.4. Can we only land this in main where toolbar will be removed?

longwave’s picture

Status: Reviewed & tested by the community » Needs review
larowlan’s picture

Status: Needs review » Needs work

Yeah I'm with @longwave on this, I was expecting to see the code from workspaces_ui moved into toolbar

gábor hojtsy’s picture

Status: Needs work » Needs review

Ok changed it to move code instead of remove code which is a 30 net line code addition apparently. 11 files +55 −22

gábor hojtsy’s picture

Title: Remove toolbar support from workspaces » Move toolbar support from Workspaces to Toolbar
Issue summary: View changes
longwave’s picture

Status: Needs review » Needs work

I think we could simplify by registering the lazy builder service always, and then it only gets used if Workspaces is installed, there is no harm in an unused service so conditionally enabling it is just an extra complication.

gábor hojtsy’s picture

Status: Needs work » Needs review

I went by how the existing NavigationServiceProvider uses the conditional pattern for navigation.workspaces_lazy_builders already, that is how it was done there :) But it does look like other core modules use a nullable argument yeah, so modified to that now.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Agreed that this approach is much more update friendly :)

  • catch committed a82a699c on main
    task: #3620812 Move toolbar support from Workspaces to Toolbar
    
    By:...
needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

Version: main » 11.x-dev
Status: Needs work » Patch (to be ported)

Committed/pushed to main, thanks!

This doesn't cherry-pick cleanly to 11.x, not sure we really need to, but if we do then will need a backport MR.

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

quietone’s picture

11.x MR available

godotislate’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

Compared 11.x MR diff to main commit diff, and they look essentially the same, other than adding autowire to the service definition.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

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.

  • catch committed 0465fa03 on 11.x
    task: #3620812 Move toolbar support from Workspaces to Toolbar
    
    By:...