Problem/Motivation

THEN: See #2489460: [Meta] Move module.theme.css files to Classy or Seven

NOW: Changes to Classy are protected by strict backwards-compatibility requirements, so we'd rather move user.icons.admin.css (user.icons.theme.css was renamed to that in #2566771: [Voltron patch] Move all remaining *.theme.css to Classy, as we discovered in #75) from the core User module to Seven.

Remaining tasks

Move user.icons.admin.css, and its library, from the User module to Seven.
Add the library in the seven twig template (?)

User interface changes

None for Seven, but Stark will be more stark.

API changes

None.

Data model changes

Files: 

Comments

Manjit.Singh created an issue. See original summary.

pguillard’s picture

Status: Active » Needs review
FileSize
755 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,903 pass(es). View

Like this ?

pguillard’s picture

FileSize
1.32 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,867 pass(es). View

Status: Needs review » Needs work

The last submitted patch, 3: move_user_icons_theme_to_seven-2566831-3.patch, failed testing.

pguillard’s picture

Status: Needs work » Needs review
FileSize
1.66 KB

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

Status: Needs review » Needs work

The last submitted patch, 5: move_user_icons_theme_to_seven-2566831-5.patch, failed testing.

pguillard’s picture

pguillard’s picture

Status: Needs work » Needs review
FileSize
0 bytes
pguillard’s picture

The last submitted patch, 8: move_user_icons_theme_to_seven-2566831-7.patch, failed testing.

Manjit.Singh’s picture

Status: Needs review » Needs work
FileSize
238.73 KB

@pguillard seems like patch is not working well on local.
I have checked a UI and the "person.svg" has not been called in css. Please check the screenshot.

issues

may be there is problem with the embedding CSS file in seven.libraries.yml

HOG’s picture

andypost’s picture

+++ 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?

Sumit kumar’s picture

andypost’s picture

Status: Needs work » Needs review

to trigger testing

andypost’s picture

Status: Needs review » Needs work

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

finnsky’s picture

finnsky’s picture

Status: Needs work » Needs review
andypost’s picture

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

trailing whitespace

finnsky’s picture

Manjit.Singh’s picture

Version: 8.0.x-dev » 8.1.x-dev
Manjit.Singh’s picture

pguillard’s picture

Patch rerolled on 8.1.x-dev.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pguillard’s picture

.. and still applies to 8.2.x

HOG’s picture

Status: Needs review » Needs work

The last submitted patch, 26: move_user_icons_theme-2566831-26.patch, failed testing.

HOG’s picture

Edited path to icon.

HOG’s picture

Issue summary: View changes
FileSize
371.39 KB

Patch works correct.
Seven toolbar user icon

HOG’s picture

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

Status: Reviewed & tested by the community » Needs review

Do not rtbc own patches

kostyashupenko’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
37.07 KB

Patch from #28 looks ok on my side
patch_28_0.png

Cottser’s picture

Title: move user.icons.theme to seven » Move user.icons.theme to Seven
Status: Reviewed & tested by the community » Needs work

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.

HOG’s picture

andypost’s picture

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

Status: Needs review » Needs work

The last submitted patch, 34: move_user_icons_theme-2566831-34.patch, failed testing.

HOG’s picture

HOG’s picture

The last submitted patch, 37: move_user_icons_theme-2566831-37.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 37: move_user_icons_theme-2566831-37.patch, failed testing.

HOG’s picture

joginderpc’s picture

Status: Needs review » Needs work
FileSize
21.44 KB

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.

path not correct

HOG’s picture

@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.

HOG’s picture

Status: Needs work » Needs review
andypost’s picture

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

joginderpc’s picture

Issue tags: +stable

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

Manjit.Singh’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update
Cottser’s picture

Title: Move user.icons.theme to Seven » Remove user.icons.theme from core CSS
Assigned: Cottser » Unassigned
Issue tags: -Needs committer feedback

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).

joginderpc’s picture

Issue tags: +move icons file, +tiny patches

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.

Cottser’s picture

Issue tags: -move icons file, -tiny patches

We don't need more tags :)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

deepakkumar14’s picture

Submit patch for this issues.
Reviews this patch.

deepakkumar14’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 52: move_user_icons_theme-2566831-42.patch, failed testing.

deepakkumar14’s picture

Status: Needs work » Needs review
FileSize
3.2 KB

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

Status: Needs review » Needs work

The last submitted patch, 55: move_user_icons_theme-2566831-43.patch, failed testing.

imshivani’s picture

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

imshivani’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 57: remove-user-icon-2566831-57.patch, failed testing.

Sumit kumar’s picture

Issue tags: +Dublin2016
pguillard’s picture

pguillard’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 61: remove-user-icon-2566831-61.patch, failed testing.

HOG’s picture

Assigned: Unassigned » HOG
HOG’s picture

Assigned: HOG » Unassigned
Status: Needs work » Needs review
FileSize
91.99 KB
3.34 KB

Rerolled patch.

Status: Needs review » Needs work

The last submitted patch, 65: move_user_icons_theme-2566831-65.patch, failed testing.

HOG’s picture

HOG’s picture

Status: Needs work » Needs review
HOG’s picture

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

The last submitted patch, 67: move_user_icons_theme-2566831-67.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 69: move_user_icons_theme-2566831-69.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

starshaped’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

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

phenaproxima’s picture

Status: Needs review » Needs work

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 :(

phenaproxima’s picture

Issue tags: +Needs reroll

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.

phenaproxima’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

The IS, she is fixed.

phenaproxima’s picture

Title: Remove user.icons.theme from core CSS » Remove user.icons.admin library from core CSS
phenaproxima’s picture

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...

edysmp’s picture

Fixed needs reroll.

Manjit.Singh’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Issue tags: -needs re

Removing bad tag.