Problem

  1. The Official Drupal 8 CSS architecture documentation states that the SMACSS category name for "skin" is "theme" in Drupal, but the corresponding PHP constant name is CSS_SKIN.

Goal

  1. Consistency.

Proposed solution

  1. Rename the constant from CSS_SKIN to CSS_THEME.
  2. Adjust + simplify drupal_get_library() accordingly.

.

.

.

Original summary:

Problem

Goal

  • Be consistent with the SMACSS standard.

Details

  • The group name "theme" is literally derived from the Official Drupal 8 CSS architecture documentation.
  • The name "theme" was chosen instead of "skin", because Drupal already has a concept of "themes", and themes want to be able to override any kind of .theme.css.
  • In addition, we introduced a formal split into foo.base.css and foo.theme.css for D7 already.
  • That naming decision is also the reason for why the corresponding CSS files are named foo.theme.css instead of foo.skin.css in D8.

Proposed solution

  1. Rename the group from "theme" to "skin" in *.libraries.yml files.
  2. Rename all corresponding CSS files from .theme.css to .skin.css.
  3. Update the CSS architecture documentation page accordingly.
CommentFileSizeAuthor
#12 interdiff.txt675 bytesmarkhalliwell
#12 die-skin-die-2202671-12.patch2.58 KBmarkhalliwell
#9 die-skin-die-2202671-9.patch2.08 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Originally when #1921610: [Meta] Architect our CSS was happening, and all of the original renames were happening, they were using skin. I, among others, pushed back and we stopped using that completely. I had no idea we forgot to rename the CSS_SKIN constant back to CSS_THEME.

So let's just rename the constant and be done with it.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +Needs themer review

Hah, that is certainly a different (opposite) fate and action for this issue. ;)

Happy to re-purpose this issue to just fix the constant name, but let's wait at least a little for themer feedback first. :-)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

If we do choose to rename the declarations, we need to fix all *.theme.css files to match.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Yes, that is part of the proposed solution in the issue summary already:

2. Rename all corresponding CSS files from .theme.css to .skin.css.

markhalliwell’s picture

markhalliwell
Primary language English
commented

I completely agree with #1. We've already established standards (CSS architecture). Just rename the constant. Skin is so... winamp.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented

If we're going to use SMACSS conventions, it makes a lot more sense to go with theme than skin.

I'm still a bit dubious that themers should essentially be required to have at least a passing familiarity with SMACSS, but if that's what's been decided, then be consistent, rather than have a weird exception to the rest of the SMACSS group names.

Since theme is a word that has a specific meaning within Drupal, I have a feeling theme.css will likely become a dumping ground for all kinds of CSS rules, rather than just rules that fall within the SMACSS concept of theme rules, particularly for themers and site builders unfamiliar with SMACSS.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented

There's a ton of discussion of skin vs theme in #1921610: [Meta] Architect our CSS, and for a lot of sensible reasons, theme was chosen. There's no need to rehash that here. Let's go with theme and move on.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Rename SMACSS group "theme" to "skin" in libraries.yml declarations » Rename CSS_SKIN constant to CSS_THEME
Component: markup » theme system
Issue summary: View changes

Alrighty. Adjusting issue title + summary accordingly. (Leaving the old intact for posterity.)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Postponed » Needs review
StatusFileSize
new die-skin-die-2202671-9.patch2.08 KB
markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Needs review » Needs work
Issue tags: -markup, -Needs themer review

This probably needs a re-roll now that #1996238: Replace hook_library_info() by *.libraries.yml file went in.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Needs work » Needs review

Ignore me.

markhalliwell’s picture

markhalliwell
Primary language English
commented
StatusFileSize
new die-skin-die-2202671-12.patch2.58 KB
new interdiff.txt675 bytes

Nope, I was right. Here's a patch.

The last submitted patch, 9: die-skin-die-2202671-9.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Reviewed the patch, does the deed. Checked no remaining outside of vendor folders.

Unless you want to tackle the 80char line limit on those lines while you're at it? In any case the re resulting patch is still RTBC.

+++ b/core/includes/common.inc
@@ -98,7 +98,7 @@
- * The default weight for CSS rules that style design components (and their associated states and skins.)
+ * The default weight for CSS rules that style design components (and their associated states and themes.)

@@ -108,9 +108,9 @@
- * The default weight for CSS rules that style skins and are not included with components.
+ * The default weight for CSS rules that style themes and are not included with components.

80 chars.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Yay, thanks all for the quick turnaround :-)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

One less DX WTF, albeit a pretty internal one :)

alexpott’s picture

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

Committed e93b685 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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