Problem/Motivation

Toolbar is being removed from core.

Steps to reproduce

Proposed resolution

  1. Move library overrides for Toolbar from Claro and Default Admin to Toolbar.
  2. Also the testing of it from system module should move.
  3. CSS files need to move.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 3614955-nr-bot_g20x93xm.txt91 bytesneeds-review-queue-bot
#4 3614955-nr-bot_sv3tno3p.txt2.19 KBneeds-review-queue-bot

Issue fork drupal-3614955

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

gábor hojtsy created an issue. See original summary.

gábor hojtsy opened merge request !16555

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
Status: Active » Needs review

Added MR with changes proposed. Also turns out default_admin was referring to a CSS file it did not even ship :) So we are fixing that as a side effect.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3614955-nr-bot_sv3tno3p.txt2.19 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Test failure in Drupal\Tests\toolbar\Functional\ToolbarClaroOverridesTest seems related.

Maybe the part that was removed from core/themes/claro/claro.info.yml needs to be added to Toolbar? I think it was you who showed me the trick of detecting the theme and loading the css for that, so maybe this needs new library definitions for that?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review

No it was a double directory name. Fixed :)

smustgrave’s picture

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

Oh nvm :) All green.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3614955-nr-bot_g20x93xm.txt91 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.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Reviewed & tested by the community

Auto-rebased without conflicts so I don't know where is the bot getting the unable to apply :)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: +11.5.0 release priority

Adding tag as depreacting Toolbar is a 11.5 release priority.

BTW there are odd CSS changes in the MR, rem to px changes, those were all generated by the pcss to css tooling by me, but I am not aware why they need to happen. I assume the css was done prior to recent coding standards updates that are now enforced.

  • catch committed d2be04f9 on 11.x 
    task: #3614955 Move library overrides for Toolbar from Claro and Default...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 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 e326830e on main 
    task: #3614955 Move library overrides for Toolbar from Claro and Default...
godotislate’s picture

godotislate
he/him
commented
Version: 11.x-dev » main
Status: Fixed » Reviewed & tested by the community

BTW there are odd CSS changes in the MR, rem to px changes, those were all generated by the pcss to css tooling by me, but I am not aware why they need to happen.

Moving toolbar.icons.theme.pcss.css from the themes to a module makes those files get excepted from the pixels to REM conversion.

See https://git.drupalcode.org/project/drupal/-/blob/main/core/scripts/css/c...

         // Prevent converting PX to REM for icon styles. These files have been
        // added to use the `postcssUrl` plugin, but aren't compatible with
        // `postcssPixelsToRem`.
        exclude: (filePath) => filePath.match(/core\/modules.*\.icons\..*\.pcss\.css$/)
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Oops, cross posted.

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.