Follow up to #2566597: [Mega patch] Move system *.theme.css files to Classy

Lewis spotted one regression. The place block button lost its left margin.

Fix this and any others the are found.

CommentFileSizeAuthor
#9 fix_any_visual-2567845-9.patch1.29 KBLewisNyman
#8 Block_layout___Site-Install.png874.19 KBLewisNyman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez created an issue. See original summary.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented 
.button:first-child, .image-button:first-child

What is the use-case for specifying first-child here? If that were removed from the Class button.css it will fix the block placement button problem.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented

What is the use-case for specifying first-child here?

You don't want to add spacing to the left of a button when it's on it's own, it will break alignment. We have some examples of this and a long discussion in #2160621: Button spacing CSS is too fragile

Is there a way to ensure that admin.css files load last without moving them into Seven? It seems like fixing the individual bugs will take a long time, and this kind of thing will mean contrib modules will have to write really specific CSS to ensure it applies.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

Is there a way to ensure that admin.css files load last...

Not if they are provided by a module. That seems to be by design, that any CSS provided by a module is grouped together and loaded before and theme CSS.

This is the rule in question, provided by the block module's block.admin.css:

.region-title .button {
    margin-left: 1em;
}

We could copy this to Classy; otherwise, we'd have to figure out some specificity that can override the first-child.

On a plus, I've been clicking around everywhere and haven't noticed any other problems. So no glaring WTFs yet.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented

Why don't we change that design for admin CSS? Any overrides for the admin UI has to load after the admin theme to ensure it takes effect without adding meaningless specificity.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

That would require an architectural change to how CSS is processed/loaded. I don't know that we can get away with that right now.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented

That's true. My recommendation in this situation is to add a wrapper div with a complete different class, and then use that to shift the placement of the block. Something like .region-title__action? That would avoid conflicts like this.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
FileSize
Block_layout___Site-Install.png874.19 KB
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Active » Needs review
FileSize
fix_any_visual-2567845-9.patch1.29 KB

Here's a patch for the block placement button.

Here's a list of all the admin css files in modules so we have a good idea where to look.

core/modules/block/css/block.admin.css
core/modules/ckeditor/css/ckeditor.admin.css
core/modules/color/css/color.admin.css
core/modules/config_translation/css/config_translation.admin.css
core/modules/content_translation/css/content_translation.admin.css
core/modules/field_ui/css/field_ui.admin.css
core/modules/file/css/file.admin.css
core/modules/filter/css/filter.admin.css
core/modules/image/css/image.admin.css
core/modules/language/css/language.admin.css
core/modules/locale/css/locale.admin.css
core/modules/menu_ui/css/menu_ui.admin.css
core/modules/node/css/node.admin.css
core/modules/system/css/system.admin.css
core/modules/user/css/user.admin.css
core/modules/views_ui/css/views_ui.admin.css
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

I'll have to try it out, but that looks like a good solution.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented
Title: Fix any visual regressions from moving the System CSS to Classy » [regression] Fix any visual regressions from moving the System CSS to Classy
gints.erglis’s picture

gints.erglis CreditAttribution: gints.erglis commented
Status: Needs review » Reviewed & tested by the community

#9 tested, no regressions found

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 10391fc and pushed to 8.0.x. Thanks!

  • alexpott committed 10391fc on 8.0.x 
    Issue #2567845 by LewisNyman, davidhernandez: [regression] Fix any...

Status: Fixed » Closed (fixed)

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