Clean up the "admin" component in Bartik

Investigated this issue a bit. First of all, there's mainly 3 different parts on current admin.css file. One of those is demo block part and that shouldn't be in admin.css anyways. On Seven theme admin.panels.css is separated so decided to separate it also here.

In fact, it's admin-panel.css on Seven so changed the name.

Added valid comments to start of file and removed extra file.

1. +++ b/core/themes/bartik/css/components/admin-panel.css
   @@ -0,0 +1,23 @@
   +div.admin-panel {
   

   We need to remove the div from the selector here: See standards

2. +++ b/core/themes/bartik/css/components/admin-panel.css
   @@ -0,0 +1,23 @@
   +.admin-panel h3 {
   

   We shouldn't be using element selectors, but rather a child class: See standards

3. +++ b/core/themes/bartik/css/components/admin-panel.css
   @@ -0,0 +1,23 @@
   +.admin-panel dt {
   ...
   +.admin-panel dd {
   

   I think these elements might be ok by SMACSS standards, but it wouldn't hurt to use classes: See standards

4. +++ b/core/themes/bartik/css/components/admin-panel.css
   @@ -0,0 +1,23 @@
   +div.admin-panel .description {
   

   Remove the div element and convert into a child class: See standards

I have reviewed just the admin-panel component, because this is a good example of a problem we are going to run into. In order to fix the CSS in Bartik, we are going to have to go back up to the module that defines the original markup and CSS and change it to fit the standards. This is the Drupal theming equivalent of going to Transylvania to kill the head vampire.

This issue died out with the mobile initiative: #1995272: [Meta] Refactor module CSS files inline with our CSS standards.

Now we have momentum under this initiative, I think we can solve these wider problems in core that Bartik can inherit. I have created a new issue that will deal with this component across Core, Seven, and Bartik. #2399939: Refactor 'admin-panel' CSS component

Now it seems that div-selector can be removed. Not sure if there has been some change on system.css or was it my bad on first place. Also decided to remove the h3 styling as it didn't look too good. Screenshots of h3. First before removing it and second one after.

Postponed until #2399939: Refactor 'admin-panel' CSS component

#2399939: Refactor 'admin-panel' CSS component was committed; the patch from #7 no longer applies.

1. +++ /dev/null
   @@ -1,34 +0,0 @@
   -/* ---------- Admin-specific Theming ---------- */
   -.path-admin #content img {
   -  margin-right: 15px; /* LTR */
   -}
   

   This was originally added for the appearance page. It seems this code is no longer necessary:

   

2. +++ /dev/null
   @@ -1,34 +0,0 @@
   -.path-admin #content .simpletest-image img {
   -  margin: 0;
   -}
   

   This code was originally added in #924158: SimpleTest test selection form markup/styling broken but currently no longer applies:

   

3. +++ /dev/null
   @@ -1,34 +0,0 @@
   -.path-admin #admin-dblog img {
   -  margin: 0 5px;
   -}
   

   This css no longer applies, since the image has been converted to a background image.
   

Great! Nice work.

+++ b/core/themes/bartik/bartik.libraries.yml
@@ -6,12 +6,14 @@ global-styling:
+      css/components/demo-block.css: {}

This is not necessary for 99.99% of page loads. Yet we're loading it on all pages.

Could we please move this into a separate library, that's only attached on the block demo page? That's also clean-up, IMO: conceptual separation makes it easier to see what is "Bartik" itself versus what is extra stuff.
Until we have #2451411-9: Add libraries-override to themes' *.info.yml, you'd still need to list bartik/demo-block as a dependency of bartik/global-styling, but at least we'd start individual components to show up, rather than having everything inside bartik/global-styling.

That way, we don't slow down these kinds of issues, but we kill two birds with one stone.

Thoughts?

@Wim Leers Good point! I'll update the patch tomorrow.

@Wim Leers Adding demo-block.css: in separate library and removed from global-styling.

system.themes_page:
  version: VERSION
  css:
    theme:
      css/components/demo-block.css: {}
  dependencies:
    - admin/appearance
    - bartik/global-styling

Also Please check If this looks ok :)

The 'demo-block.css' file isn't added. Also, if I create it from previous patch, declaring the library "system.themes_page:" doesn't loads it. I'll work on that.

If we want the file demo-block.css in the blocks demo page, declaring the library "system.themes_page" won't load it because it's for the Appearence admin page (admin/appearance).

We could load it everywhere, or we could find a way to declare it only in that 'demo' page. This page is created in core/modules/block/src/Controller/BlockController.php for all themes.

We're not sure which is the correct way, so here's a suggestion:

First, add a way for all themes to access it in core/modules/block/src/Controller/BlockController.php, line 74:

    drupal_alter('block_controller_demo_page', $page);

Then, add the library in core/themes/bartik/bartik.theme:

function bartik_block_controller_demo_page_alter(&$page) {
  $page['#attached']['library'][] = 'block.demo_page';
}

It would let us add the library in bartik.libraries.yml as "block.demo_page" instead of "system.themes_page" .

Any idea about the best way to solve it?

Settings to needs work based on @ckrina's comments. Also the patch in #18 is missing the demo-block.css patch

If #939462: Specific preprocess functions for theme hook suggestions are not invoked gets in, you can use a specific preprocess function to attach the libraries. If not either you have to do this in hook_preprocess_page or add a overriding template which can extend the page.html.tpl and then just attach the library there. If you use second option, please create a follow-up to remove it and mark the template @deprecated so it can be done after release.

Based on the conversation I had with @lauriii I think we are are better off reducing the scope of this issue and creating a follow up issue to deal with the conditional loading of these files. We've removed a lot of CSS in this issue which is a big win. Here is the reroll. I think this patch is ready to go.

Tested this manually and there is no visual regressions in the places where these classes are applied. I tested block demo page, simpletest page and appearance page. No other changes than what's being agreed on #11.

Committed 253060b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.