Problem/Motivation

The styling of the admin-panel is not inline with our CSS coding standards.

Proposed resolution

Bring them inline with our standards.

Remaining tasks

Review the current CSS — What to look for when reviewing CSS

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because coding standards
Unfrozen changes Unfrozen because it only changes HTML/CSS
CommentFileSizeAuthor
#82 2399939-82.patch6.08 KBidebr
#82 interdiff-82-75.txt1002 bytesidebr
#82 2399939-82-after.png255.54 KBidebr
#82 2399939-82-before.png212.6 KBidebr
#77 Screen Shot 2015-02-27 at 3.58.17 PM.png98.45 KBidebr
#75 clean-up-admin-panel-2399939-75.patch5.91 KBLewisNyman
#75 Screenshot 2015-02-23 17.32.01.jpg495.54 KBLewisNyman
#75 Screenshot 2015-02-23 17.31.39.jpg496.34 KBLewisNyman
#73 clean-up-admin-panel-2399939-73.patch5.94 KBLewisNyman
#73 interdiff.txt2.56 KBLewisNyman
#67 interdiff.txt2.73 KBmherchel
#67 2399939-refactor-admin-component-67.patch6.31 KBmherchel
#64 interdiff.txt2.16 KBmherchel
#64 2399939-refactor-admin-component-64.patch5.5 KBmherchel
#60 interdiff-2399939-57-60.txt533 bytesDickJohnson
#60 clean-up-admin-panel-2399939-60.patch4.75 KBDickJohnson
#60 bartik-admin-panel-after.png65.42 KBDickJohnson
#60 bartik-admin-panel.png61.86 KBDickJohnson
#57 2399939-55-57.txt399 bytesDickJohnson
#57 clean-up-admin-panel-2399939-57.patch4.71 KBDickJohnson
#55 interdiff-2399939-52-55.txt380 bytesDickJohnson
#55 clean-up-admin-panel-2399939-55.patch4.73 KBDickJohnson
#50 2399939-49-after.png129.07 KBidebr
#50 2399939-49-before.png153.4 KBidebr
#49 clean-up-admin-panel-2399939-49.patch4.73 KBDickJohnson
#46 clean-up-admin-panel-2399939-46.patch1.15 KBbrahmjeet789
#45 interdiff-2399939-43-45.txt1.53 KBDickJohnson
#45 clean-up-admin-panel-2399939-45.patch4.65 KBDickJohnson
#43 interdiff-2399949-37-43.txt718 bytesDickJohnson
#43 clean-up-admin-panel-2399939-43.patch3.44 KBDickJohnson
#37 interdiff-2399949-34-37.txt815 bytesDickJohnson
#37 clean-up-admin-panel-2399939-37.patch3.31 KBDickJohnson
#33 interdiff-2399939-31-34.txt1.62 KBDickJohnson
#33 clean-up-admin-panel-2399939-34.patch3.46 KBDickJohnson
#31 clean-up-admin-panel-2399939-31.patch3.51 KBDickJohnson
#28 interdiff-2399939-25-26.txt619 bytesDickJohnson
#28 admin-panel-markup-new.png65.52 KBDickJohnson
#28 clean-up-admin-panel-2399939-26.patch3.65 KBDickJohnson
#25 interdiff-2399939-23-25.txt617 bytesDickJohnson
#25 clean-up-admin-panel-2399939-25.patch3.65 KBDickJohnson
#25 clean-up-admin-panel-2399939-25.patch3.65 KBDickJohnson
#23 interdiff-2399939-20-23.txt836 bytesDickJohnson
#23 clean-up-admin-panel-2399939-23.patch3.65 KBDickJohnson
#23 admin-panel-after-templ-refactor.png68.8 KBDickJohnson
#23 admin-panel-markup-after-refactor.png59.73 KBDickJohnson
#20 clean-up-admin-panel-2399939-20.patch3.51 KBDickJohnson
#19 clean-up-admin-panel-2399939-19.patch2.83 KBDickJohnson
#9 clean-up-admin-panel-2399939-9.patch2.91 KBLewisNyman
#8 2399939-7-annotated-after.png167.68 KBidebr
#8 2399939-7-annotated-before.png134.18 KBidebr
#7 interdiff-23999939-4-7.txt635 bytesDickJohnson
#7 clean-up-admin-panel-2399939-7.patch2.49 KBDickJohnson
#6 Screenshot 2015-01-06 22.47.46.jpg580.47 KBLewisNyman
#6 Screenshot 2015-01-06 22.47.30.jpg513.29 KBLewisNyman
#6 Screenshot 2015-01-06 22.47.05.jpg559.35 KBLewisNyman
#4 clean-up-admin-panel-2399939-4.patch2.5 KBDickJohnson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

I'm not even sure if admin-panel is the correct name here, components should be generic irrespective of their content.

DickJohnson’s picture

By template name and comments it's been called as admin blocks, but I'm not sure if that's a correct name either as technically these things are not drupal blocks.

DickJohnson’s picture

Current css on modules/system/css/system.admin.css is relatively good for this particular element. There's only few rows:

/**
 * Administration blocks.
 */
.admin-panel {
  margin: 0;
  padding: 5px 5px 15px 5px;
}
.admin-panel .description {
  margin: 0 0 3px;
  padding: 2px 0 3px 0;
}

Rest of the stuff is done either in Seven or in Bartik. I'd like to change the template so that we would have some reusable components for Seven and Bartik. The template could look something like:

modules/system/templates/admin-block-html.twig

<div class="admin-panel">
  {% if block.title %}
    <h3 class="admin-panel__title">{{ block.title }}</h3>
  {% endif %}
  {% if block.content %}
    <div class="body admin-panel__body">{{ block.content }}</div>
  {% elseif block.description %}
    <div class="description admin-panel__description">{{ block.description }}</div>
  {% endif %}
</div>

I'm not completely sure if this is appropriate or should that be done both in Seven and in Bartik. But f.ex for Bartik it would change the look of css to:

.admin-panel {
  background: #fbfbfb;
  border: 1px solid #ccc;
  margin: 10px 0;
  padding: 0 5px 5px;
}
.admin-panel__title {
  margin: 16px 7px;
}
.admin-panel__description {
  margin: 0 0 14px 7px;
}
.admin-panel dt {
  border-top: 1px solid #ccc;
  padding: 7px 0 0;
}
.admin-panel dd {
  margin: 0 0 10px;
}

Which is starting to look relatively good. dt and dd are still a bit of a problem but I'm not sure if there's much we can do about it.

DickJohnson’s picture

Status: Active » Needs review
FileSize
2.5 KB

So, I think is the only way to get feedback.

idebr’s picture

The css looks spot on, a sight for sore eyes :)

+++ b/core/modules/system/templates/admin-block.html.twig
@@ -16,11 +16,11 @@
+    <div class="body admin-panel__body">{{ block.content }}</div>

Are you leaving the original body class in for compatibility? A pure SMACCS implementation would suggest just admin-panel__body instead of body admin-panel__body.

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: +d8mux-css-cleanup
FileSize
559.35 KB
513.29 KB
580.47 KB

Yep I think we should get rid of the body and description classes.

Apart from that, it looks good. I manually tested the patch in Classy, Seven, and Bartik. It looks the same.

DickJohnson’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
635 bytes

Removed body and description -classes.

idebr’s picture

Status: Needs review » Needs work
FileSize
134.18 KB
167.68 KB

The component looks much cleaner now :)

I found one difference in Seven, since Seven has a custom template for admin-block-content.html.twig that needs to be updated with the new class.

Screenshot before:

Screenshot after:

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

Oh nice catch. This is confusing. The description class isn't part of the admin-panel component, it's part of the admin-list component. Why do we have two components? I guess it doesn't hurt...

What we can do is move the selector into the admin-list.css file and rename admin-panel to admin-list. Then it fixes the minor regression and puts it where it belongs. I also renamed the dt and dd selectors in Bartik to match this.

I also went through and found some CSS that wasn't being used, and tidied up a few properties.

DickJohnson’s picture

Two components doesn't really hurt on this case as another one is for content and another one for the rest. Crappy thing is that by name no-one knows what admin-panel and admin-list are. I'd rename those as admin-block and admin-block__content.

LewisNyman’s picture

Admin block sounds just as generic as admin panel. Panel at least is a convention: Bootstrap, Foundation

LewisNyman’s picture

I have problems with it being called admin-* though. What's do adminy about it? We don't name another of our other UI components like this.

idebr’s picture

Status: Needs review » Needs work

I agree with @LewisNyman the admin-* part seems superfluous. Let's take a step back and see if we can split this into seperate components.

Candidate names per component:

- panel (Bootstrap / Foundation)
- list-group (Bootstrap) / block-list (Inuit)
- list-group-item (Boostrap) / complex-link (Inuit) / link-group (idebr)

Current hierarchy:

div.admin-panel
div.admin-panel .body

ul.admin-list
.admin-list li
.admin-list li a
.admin-list li a .label
.admin-panel .description

Suggested hierarchy:

.panel
.panel__title
.panel__body

.list-group
.list-group__item

.link-group
.link-group__label
.link-group__description

Current markup:

<div class="admin-panel">
  <h3>People</h3>
  <div class="body">
    <ul class="admin-list">
      <li>
        <a href="/admin/config/people/accounts">
          <span class="label">Account settings</span>
          <div class="description">Configure default behavior of users, including registration requirements, emails, and fields.</div>
        </a>
      </li>
    </ul>
  </div>
</div>

Suggested markup:

<div class="panel">
  <h3 class="panel__title">People</h3>
  <div class="panel__body">
    <ul class="list-group">
      <li class="list-group__item">
        <a href="/admin/config/people/accounts" class="link-group">
          <span class="link-group__label">Account settings</span>
          <div class="link-group__description">Configure default behavior of users, including registration requirements, emails, and fields.</div>
        </a>
      </li>
    </ul>
  </div>
</div>
LewisNyman’s picture

List group is a little generic for my tastes, I just noticed the Seven style guide defines it as nav list. Will post more detailed feedback in a bit. We also have a separate issue somewhere for creating a reusable 'panel' component. #2113903: Introduce a 'pane' component to visually group form items

LewisNyman’s picture

I think panel__content instead of panel__body would fit closer to the block terminology we use, what do you think?

I noticed we are switching to three components from two. It feels like it could be too much, here's a proposal with two components. This fits with my interpretation of SMACSS, which doesn't consider on HTML structure.

<?php
<div class="panel">
  <h3 class="panel__title">People</h3>
  <div class="panel__body">
    <ul class="list-group">
      <li class="list-group__item">
        <a href="/admin/config/people/accounts">
          <span class="list-group__label">Account settings</span>
          <div class="list-group__description">Configure default behavior of users, including registration requirements, emails, and fields.</div>
        </a>
      </li>
    </ul>
  </div>
</div>
?>
DickJohnson’s picture

I'm fine with that one.

idebr’s picture

@LewisNyman

I think panel__content instead of panel__body would fit closer to the block terminology we use, what do you think?

Agreed

List group is a little generic for my tastes, I just noticed the Seven style guide defines it as nav list.

In #15 you suggest .list-group for the <ul>, did you change your mind? Either is fine by me.

I noticed we are switching to three components from two. It feels like it could be too much, here's a proposal with two components. This fits with my interpretation of SMACSS, which doesn't consider on HTML structure.

Indeed I suggested to use 3 components instead of 2:

  1. .panel (I think this one is uncontested)
  2. .list-group
  3. .link-group

If we define __ as a child, then in a pure implementation the class names should be:

<ul class="list-group">
  <li class="list-group__item">
    <a href="/admin/config/people/accounts" class="list-group__item__link">
      <span class="list-group__item__link__label">Account settings</span>
      <div class="list-group__item__link__description">Configure default behavior of users, including registration requirements, emails, and fields.</div>
    </a>
  </li>
</ul>

This makes the classes very verbose.

Note: we also need a class on the <a> for the css that is currently being applied to .admin-list li a if we wish to keep the styling seperated from the markup.

I based the maximum of 2 levels of nesting on the Sass modules styleguide: https://gist.github.com/trinonsense/8295577#module-component:

Avoid more than 2 levels of component:

  • long class selectors can occur
  • it creates a restriction on module complexity; maintaining modularity
  • if a module requires more than 2 levels of components, create different modules for those components

One could argue either way, but two components could work just as well. @LewisNyman, I'll leave this one up to you and work on a patch when the decision is in. Either way this case should be document on the CSS coding standards page for later reference: https://www.drupal.org/node/1887918

LewisNyman’s picture

But we don't need to mimic the nesting of HTML in our component selectors, so list-group__item__link__label and list-group__item__link__description can just be list-group__item__label and list-group__item__description. Does that make sense?

DickJohnson’s picture

Started to work on this. What still needs to be done:
1. Split the {{ item.link }} up in admin-block-content.html.twig
2. Implement the changes to Seven
3. Fix .list-group in Bartik
4. Rename the files?

DickJohnson’s picture

Implemented stuff to seven.

Remaining things to do:
1. Split the {{ item.link }} up in admin-block-content.html.twig
2. Fix .list-group in Bartik and Seven
3. Rename the files?

DickJohnson’s picture

Current markup by idebr on #13 is not actually current markup from system module, it's current markup from Seven theme. Current markup from system.module outputs the item.link as <a href="foo/bar">Blaa</a> without any spans.

LewisNyman’s picture

That's fine, let's just use those sub-items in Seven but don't use it elsewhere

DickJohnson’s picture

Finalized the template refactoring. Decided to split the {{ item.link }} anyways, as I don't think it's good idea to output mixes of links and labels. Didn't go with suggested markup as it would have changed the functionality a bit. So ended up with:

Markup

Atleast Bartik's admin panel is looking pretty ok after this:
Bartik

DickJohnson’s picture

Status: Needs work » Needs review

Right. 2 mins after sending the patch I can see that there's one false class on link.

DickJohnson’s picture

The last submitted patch, 23: clean-up-admin-panel-2399939-23.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: clean-up-admin-panel-2399939-25.patch, failed testing.

DickJohnson’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
65.52 KB
619 bytes

And once more:

Markup

The last submitted patch, 25: clean-up-admin-panel-2399939-25.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 28: clean-up-admin-panel-2399939-26.patch, failed testing.

DickJohnson’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

After a short chat in irc with @lewisnyman reverting back to {{ item.link }}.

Things still to do:
1. On Bartik theme ".region content ul" and "block ul" are overriding margins on .list-group. Everything else seems to be fine. Those could be handled on Bartik cleanup issues also.

LewisNyman’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/css/system.admin.css
    @@ -47,11 +47,11 @@
    +.panel {
       margin: 0;
    

    This margin does nothing, we can delete it.

  2. +++ b/core/modules/system/css/system.admin.css
    @@ -47,11 +47,11 @@
       padding: 5px 5px 15px 5px;
    

    I think we can remove the final unit and change this to padding: 5px 5px 15px;

  3. +++ b/core/modules/system/templates/admin-block-content.html.twig
    @@ -18,17 +18,17 @@
    -  <dl{{ attributes.addClass(classes) }}>
    +  <ul{{ attributes.addClass(classes) }}>
    ...
    -      <dt>{{ item.link }}</dt>
    +      <ul class="list-group__item">{{ item.link }}</ul>
    ...
    -        <dd>{{ item.description }}</dd>
    +        <div class="list-group__item__description">{{ item.description }}</div>
    

    These should be changed back to dl, dt, and dd. I think they are like that doe accessibility reasons

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -18,17 +18,17 @@
+        <div class="list-group__item__description">{{ item.description }}</div>

+++ b/core/themes/bartik/css/components/admin.css
@@ -28,22 +28,24 @@
+.list-group__item__description {

We can just name this class list-group__description. See the best practices - https://www.drupal.org/node/1887918#sub-components

DickJohnson’s picture

Made the changes suggested by @lewisnyman on #32.

DickJohnson’s picture

Status: Needs work » Needs review
LewisNyman’s picture

We are getting there! Thanks for the hard work.

  1. +++ b/core/modules/system/templates/admin-block-content.html.twig
    @@ -18,16 +18,16 @@
    -      <dt>{{ item.link }}</dt>
    +      <dt class="list-group__link">{{ item.link }}</dt>
    
    +++ b/core/themes/bartik/css/components/admin.css
    @@ -28,22 +28,24 @@
    -div.admin-panel dt {
    +.panel__content {
    

    This css used to be selecting the dt, but now it's selecting a different element, because the dt now has the class panel__link, not .panel__content

  2. +++ b/core/themes/seven/css/components/admin-panel.css
    @@ -1,20 +1,23 @@
    +.panel,
    +.panel__content {
       padding: 0;
       clear: left;
     }
    

    I think we can delete this CSS, padding iss 0 by default anyway so we don't need that. I don't know what clear left does here. Let's try deleting it and seeing what comes up

  3. +++ b/core/themes/seven/css/components/admin-panel.css
    @@ -1,20 +1,23 @@
    +.list-group__item {
    +  list-style: none;
    +}
    

    This class no longer exists

LewisNyman’s picture

Status: Needs review » Needs work
DickJohnson’s picture

Ok, the reason why I didn't notice #35.1 is that it's not actually doing anything difference if it's in .panel__content, .list-group__link or if it doesn't exist, so I removed it.

2 and 3 are fixed.

DickJohnson’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Issue summary: View changes
idebr’s picture

DickJohnson’s picture

Status: Needs work » Needs review

I'm not touching Seven on this patch at all as it's handling it's own admin-panel structure on template level.

LewisNyman’s picture

Status: Needs review » Needs work

I'm happy to spin out Seven into a separate issue.

  1. +++ b/core/modules/system/css/system.admin.css
    @@ -47,11 +47,10 @@
     /**
      * Administration blocks.
      */
    

    We should update this comment. How about:
    "Panel.
    Used to visually group items together."

  2. +++ b/core/themes/seven/css/components/admin-panel.css
    @@ -1,18 +1,13 @@
     /**
      * Admin panel.
      */
    

    Same situation here, in fact we should rename this CSS file to panel.css.

I think we can leave moving around the CSS files in Bartik for #2398453: Clean up the "admin" component in Bartik

DickJohnson’s picture

Status: Needs work » Needs review
FileSize
3.44 KB
718 bytes

Seven's admin-panel.css was missing file-comment completely so decided to do it that way. 42.1 fixed as suggested.

LewisNyman’s picture

Status: Needs review » Needs work

Thanks, the only thing left to do here is to rename the Seven file from admin-panel.css to panel.css

DickJohnson’s picture

Status: Needs work » Needs review
FileSize
4.65 KB
1.53 KB

Renamed.

brahmjeet789’s picture

Changed the admin-pannel.css o pannel.css

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

@brahmjeet789 I think you might of uploaded the wrong patch?

The patch in #45 looks good though. It needs a reroll now though :(

brahmjeet789’s picture

Thanks @LewisNyman, yup the patch #45 is working fine .

DickJohnson’s picture

idebr’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll
FileSize
153.4 KB
129.07 KB

Seven has its own implementation for the panel content, but the switch from .admin-panel to .panel does affect Seven. For comparison:

DickJohnson’s picture

It seems that I forgot to apply new classes to Sevens panel.css on reroll.

DickJohnson’s picture

Status: Needs work » Needs review
FileSize
4.7 KB
570 bytes

That should do it.

LewisNyman’s picture

Status: Needs review » Needs work
LewisNyman’s picture

  1. +++ b/core/modules/system/css/system.admin.css
    @@ -45,13 +45,14 @@
    -.admin-panel {
    +.panel {
       margin: 0;
    

    We don't need margin 0 here.

  2. +++ b/core/modules/system/css/system.admin.css
    @@ -45,13 +45,14 @@
       padding: 5px 5px 15px 5px;
    

    We also don't need this last 5px unit here.

DickJohnson’s picture

Status: Needs work » Needs review
FileSize
4.73 KB
380 bytes

Fixed #54.1 and #54.2.

idebr’s picture

Status: Needs review » Needs work
  1. +++ b/core/themes/seven/css/components/panel.css
    @@ -0,0 +1,20 @@
    +/**
    + * Admin panel.
    + */
    

    This comment should be updated to reflect the new name of the component. I suppose it could be the same as the system.admin.css. This file should also include an @file docblock.

  2. +++ b/core/themes/seven/css/components/panel.css
    @@ -0,0 +1,20 @@
    +.panel,
    +.panel__body {
    +  padding: 0;
    +  clear: left;
    +}
    

    This css was removed after 35.2, but the change was lost somewhere along the way. This can still be removed.

DickJohnson’s picture

Fixed #56.1 and #56.2.

DickJohnson’s picture

Status: Needs work » Needs review
idebr’s picture

Status: Needs review » Needs work
  1. The feedback from #8 has not yet been addressed
  2. +++ b/core/modules/system/templates/admin-block-content.html.twig
    @@ -18,16 +18,16 @@
       set classes = [
    -    'admin-list',
    +    'list-group',
         compact ? 'compact',
    ...
       <dl{{ attributes.addClass(classes) }}>
    
    +++ b/core/themes/bartik/css/components/admin.css
    @@ -31,26 +31,22 @@
    -div.admin-panel dt {
    +.list-group {
    

    There is a mismatch in the new class for Bartik. Previously the styling was applied to the <dt>, while in the patch the styling is applied to the <dl>. This introduces a visual regression.

  3. +++ b/core/themes/bartik/css/components/admin.css
    @@ -31,26 +31,22 @@
    -div.admin-panel .description {
    -  margin: 0 0 14px 7px; /* LTR */
    -}
     [dir="rtl"] div.admin-panel .description {
       margin-left: 0;
       margin-right: 7px;
    

    This styling was historically applied to the <dd> tag, but currently does not match any markup. Let's correct this styling to it applies again.

DickJohnson’s picture

Fixed the issues mentioned in #59.

Before:
Before

After:
After

DickJohnson’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work

Thanks for the hard work here, this is a big improvement.

+++ b/core/modules/system/templates/admin-block.html.twig
index bb1d6ed..04c0038 100644
--- a/core/themes/bartik/css/components/admin.css

--- a/core/themes/bartik/css/components/admin.css
+++ b/core/themes/bartik/css/components/admin.css

+++ b/core/themes/bartik/css/components/admin.css
+++ b/core/themes/bartik/css/components/admin.css
@@ -31,27 +31,23 @@

@@ -31,27 +31,23 @@
   width: 500px;
 }
...
-/* Configuration. */
-div.admin-panel {
+.panel {
   background: #fbfbfb;
   border: 1px solid #ccc;
   margin: 10px 0;
   padding: 0 5px 5px;
 }
-div.admin-panel h3 {
+.panel__title {
   margin: 16px 7px;
 }
-div.admin-panel dt {
+.list-group__link {
   border-top: 1px solid #ccc;
   padding: 7px 0 0;

All that is left to do is to move these components out of admin.css and into panel.css and list-group.css

mherchel’s picture

Issue tags: +LatinAmerica2015

Working on this now in Bogota

mherchel’s picture

Status: Needs work » Needs review
FileSize
5.5 KB
2.16 KB

Patch and interdiff attached!

DickJohnson’s picture

Status: Needs review » Needs work

Looks like you forgot to add Sevens panel.css and Bartiks list-group.css file.

mherchel’s picture

Yep. Fixing now :)

mherchel’s picture

Status: Needs work » Needs review
FileSize
6.31 KB
2.73 KB

Fixed

DickJohnson’s picture

Everything else seems to be okay but in:

1. bartik/css/components/admin.css

+[dir="rtl"] .admin-panel__description {
   margin-left: 0;
   margin-right: 7px;
 }

That's somehow wrong. Should be in panel.css, right?

2. Seven's panel.css is stil missing.

DickJohnson’s picture

Status: Needs review » Needs work
idebr’s picture

Sevens panel.css is still missing

+++ b/core/themes/bartik/bartik.libraries.yml
@@ -7,6 +7,7 @@ global-styling:
+      css/components/buttons.css: {}

Some code from bartiks button.css seems to be leaking

DickJohnson’s picture

button.css is removed from bottom of libraries.yml to upper parts of it yes. That might cause possible problems.

The last submitted patch, 67: 2399939-refactor-admin-component-67.patch, failed testing.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
2.56 KB
5.94 KB

I gave the patch another go

DickJohnson’s picture

Status: Needs review » Needs work
-div.admin-panel .description {
-  margin: 0 0 14px 7px; /* LTR */
-}
-[dir="rtl"] div.admin-panel .description {
-  margin-left: 0;
-  margin-right: 7px;
-}

Are these unnesessary?

LewisNyman’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
496.34 KB
495.54 KB
5.91 KB

Here are before/after screenshots of the panels in Bartik, you tell me ;-)


Also, the patch before was missing a blank line in a file. Here is the fixed patch.

DickJohnson’s picture

Status: Needs review » Reviewed & tested by the community

Checked the patch and it's great.

idebr’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
98.45 KB

@LewisNyman In 59.2 and 59.3 I suggested to fix the Bartik admin-panel selectors instead of removing the styles, since they historically created a much nicer admin-panel. For reference a screenshot of the Bartik admin-panel in Drupal 7:

LewisNyman’s picture

@idebr Ah yes, sorry I missed that/

DickJohnson’s picture

I think it should be handled it follow-up issue. We haven't done any stylistical changes in these refactor/write css per standards issues. It is a valid point anyways.

DickJohnson’s picture

Status: Needs work » Reviewed & tested by the community
idebr’s picture

Assigned: Unassigned » idebr
Status: Reviewed & tested by the community » Needs work

Actually, in the case of a broken selector we historically fixed the selector instead of removing the styling, for example:
- #1663210: Clean up css in the User module
- #2398461: Clean up the "comments" component in Bartik

With our current infrastructure it is very easy to introduce regressions without anyone noticing. When this happens we should fix it sooner rather than later and certainly not remove any styling that applied previously, since it is very likely we will lose track of it entirely.

I understand you would like to get this committed, so I'll work on a patch to bring the visual styling back to the admin panels.

idebr’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
212.6 KB
255.54 KB
1002 bytes
6.08 KB

The missing styling was actually already removed; it used to live in system.admin.css in Drupal 7. I copied the styling over from Drupal 7 to fix the visual regression.

The styling that was removed by @LewisNyman in #75 was actually never used, not even in Drupal 7.

Screenshot before:

Screenshot after:

idebr’s picture

Assigned: idebr » Unassigned
DickJohnson’s picture

Status: Needs review » Reviewed & tested by the community

Checked the patch, it's applying and fixing the coding standard issues. By what @idebr pointed out in #81 setting this as RTBC even it's changing the look of Bartik's panel a lot.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. I'm not 100% sure of this change. Aren't we worried about namespace collisions with Panels module in contrib?

idebr’s picture

I checked the current Panels 8.x-3.x codebase for naming conflicts. Panels prefixes its class names with .panels-* and an individual panel is called a .panels-pane. There is no direct naming conflict and even the namespace is available.

Arguments for .panel:

  1. The naming is consistent with similar components in other CSS frameworks, such as Bootstrap / Foundation
  2. The current name 'admin-panel' in incorrect is, components should be generic irrespective of their content.
  3. Does not directly conflict with naming in the Panels contrib module.

Arguments against .panel:

  1. Themers might use .panel in custom styling
  2. Site builders might confuse .panel with a Panel contrib component

I think we're safe on against #2, since .panel is used on a very specific set of pages that are not commonly extended with Panel (contrib) content. I'm not sure on against#1, perhaps someone who actually uses Panels can comment on it?

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

@idebr Thanks that was a more detailed answer than I would of given :). I don't think argument #1 against panel is a concern, you could say that about any class really, the process would be the same: remove or override existing system CSS.

alexpott’s picture

So @idebr's comment in #8 will be addressed in a followup? Has one been created?

idebr’s picture

@alexpott The selector collision has been in HEAD since Drupal 7. Now that we have proper components, I'm not sure if this collision was intended or not. I have created a followup and postponed it on this issue so we can keep moving forward: #2449555: Check design for panels (admin/config) with the Seven style guide

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

CSS is not frozen in beta. Committed c016165 and pushed to 8.0.x. Thanks!

  • alexpott committed c016165 on 8.0.x
    Issue #2399939 by DickJohnson, LewisNyman, mherchel, idebr, brahmjeet789...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

brahmjeet789’s picture

brahmjeet789’s picture