Remove user.icons.admin library from core CSS

Like this ?

I wonder if this is still needed ?
Here is a re-rolled patch

+++ b/core/themes/seven/seven.libraries.yml
+++ b/core/themes/seven/seven.libraries.yml
@@ -31,6 +31,7 @@ global-styling:

@@ -31,6 +31,7 @@ global-styling:
+      css/components/user.icons.admin.css: {}

do we really need that on each page?

Adding Patch for this

to trigger testing

this should extend toolbar library so loaded only when toolbar module enabled

added condition for this css.
same idea as in https://www.drupal.org/node/2566855#comment-10823376

+++ b/core/themes/seven/seven.info.yml
@@ -41,6 +41,8 @@ libraries-extend:
+  toolbar/toolbar:
+    - seven/seven.toolbar    ¶

trailing whitespace

fixed

Patch rerolled on 8.1.x-dev.

.. and still applies to 8.2.x

Edited path to icon.

Patch works correct.

Do not rtbc own patches

Patch from #28 looks ok on my side

Thanks everyone who has worked on this so far. The patch here is not moving, it's copying (doesn't match up with title/issue summary).

I don't see it being discussed in this issue but we could potentially remove the CSS entirely from the user module as long as we ensure the Stable version is still loaded. That's one of the main purposes of the Stable theme – to provide a backwards compatibility layer.

This break a lot of things

core8$ git grep -F 'drupal.user.icons'
core/modules/user/user.libraries.yml:30:drupal.user.icons:
core/modules/user/user.module:1382:        'user/drupal.user.icons',
core/themes/stable/stable.info.yml:223:  user/drupal.user.icons:

1. +++ b/core/modules/user/user.libraries.yml
   @@ -26,9 +26,3 @@ drupal.user.permissions:
   -drupal.user.icons:
   

   all themes that override this library will fail

2. +++ b/core/themes/seven/seven.libraries.yml
   @@ -25,6 +25,7 @@ global-styling:
   +      css/components/user.icons.admin.css: {}
   

   unneeded

It seems that we are not on right track to moving the css to Seven theme. As mentioned in Screenshot the path is showing for Stable theme. Please check.

@joginderpc, check pls https://www.drupal.org/node/2566831#comment-11126239.
If we move code to Seven we have issue in Bartik & another themes - icon in toolbar not display. As this themes are subthemes for Stable, icons in stable theme works ok & we not need to copy this code to each theme.

Patch makes sense to me
Stable actually the one that used so makes no sense to keep the same file in user module.

So probably we need to re-title issue subject cos no actual changes are happen except removal styling from user module.
Suppose @Cottser should decide here

Ya that's fine @HOG and yes now the title need to be changed @Cottser need to help on this.

I don't quite follow the logic here anymore, maybe an issue summary update would help indeed :)

Based on what we did in #2566771: [Voltron patch] Move all remaining *.theme.css to Classy I have a couple thoughts:

I would say it makes more sense (based on what we were doing before 8.0.0 and the fact that we have Stable now) to remove the CSS from core leaving the library intact and leave Stable alone. But this assumes that we actually do want to remove all toolbar icons from the core CSS and this decision shouldn't be based on the fact that it's loading from Stable now. No CSS is loaded directly from core modules unless you go base theme-less which means you don't get backwards compatibility. Based on this train of thought I'm tentatively retitling the issue.

Regarding issue scoping this change seems like it's too small. If we go along the route of removing all the toolbar icons from the core CSS I think we'd be committing lots of tiny patches and potentially leaving the toolbar in a very weird state (in core sans base theme). There's also shortcut's star icon and such. If we want to keep pursuing the removal of the icons from the toolbar in core CSS I think we need a larger patch or we need to Voltron again (meaning get a bunch of small patches ready and roll them together into one patch).

Yes i agree with @cottser we need to make tiny patches and club them in one big patch so the tester also track the changes.

We don't need more tags :)

Submit patch for this issues.
Reviews this patch.

I have submitted new patch for this.
Please review this.

Submitted patch for the issue. You can review the patch.

Should pass tests

Rerolled patch.

I rerolled Pguillard's patch. I think it's good decision.

Removed reference to drupal.user.icons in user.libraries.yml as this was causing the kernel test to fail.

Thank you, @starshaped, for fixing the tests!

Unfortunately, I gave this a quick manual test and discovered that it still appears to be broken, in that there is no user icon in the toolbar when I switch into Seven with this patch applied. So that will need to be addressed :(

Did some git bisect work on this with @starshaped and @xjm at DrupalCamp Montreal.

It turns out this will need to be re-rolled because user.icons.theme.css was renamed to user.icons.admin.css, in 5cb0783 (#2566771: [Voltron patch] Move all remaining *.theme.css to Classy). And it definitely needs an issue summary update.

The IS, she is fixed.

Okay, something here is strange.

My current understanding is that the following things need to happen -- based on a heady combination of the BC policy for the core themes, and the current way that things seem to be put together:

1. Remove user.icons.admin.css and its library definition from User.
2. Remove all references to that library from User (currently only user_toolbar() makes mention of it).
3. Replace Stable's libraries-override reference to user/drupal.user.icons with an actual library definition.
4. Copy user.icons.admin.css, and the library definition, to Seven and Bartik separately, duplicating the code for each theme.
5. Implement hook_toolbar_alter() in Seven, Stable and Bartik to re-add the library, effectively preserving BC for Stable.
6. Possibly write tests for all this rigmarole.

I'd like a front-end maintainer to confirm this understanding before anything further happens here...

Fixed needs reroll.

Removing bad tag.

Re-roll the patch for d9.

Hi @bandanasharma,

Patch is applied after applied patch icon is gone in drupal 9.

Thanks!

The patch doesn't look right to me. The patch should be moving the files to Seven and adding a new library definition there.

Additionally, rather than removing the definition from user module, it should be deprecated for removal in Drupal 10.

Hi @bandanasharma,

Patch applied successfully
After patch icon is gone
Thanks for the patch
For ref sharing screenshots .....

Removing credit for #94. Screenshots were already provided in #90. Both sets of screenshots are also used as evidence of success, but that's based on a misunderstanding of the issue requirements - the goal is not to remove the user icon from the toolbar, it's to remove it as a core library, and move it to a theme.

Based on the current issue summary, this is also likely unnecessary as core themes are now decoupled from Classy. This should be updated if there's an issue to ba addressed.