Problem/Motivation

As detailed in #3050389: [META] Remove dependency to Classy from core themes, Classy will be moved to contrib befire Drupal 10. Before that, we have to remove dependencies on Classy from all core themes.

Part of this process is creating theme-specific copies of any templates and CSS files that were previously inherited from Classy. There should be an easy way to distinguish these used-to-be-Classy templates/CSS files from ones created specifically for a theme.

Proposed resolution

In the templates & css directory for every core theme subtheming Classy, add a classy directory with a README explaining that:
- It is for templates/css copied from Classy,
- If a template in this directory is altered it should be moved outside the classy directory as it's now theme-specific.

Remaining tasks

User interface changes

API changes

Bartik, Umami, Seven and Claro will no longer extend Classy. We will be copying templates from Classy to these themes meaning that markup and CSS will remain untouched.

All of the themes are internal, so they shouldn't be extended. If a theme is extending one of these themes, and they also extend Classy templates or target Classy libraries with library extend or override, they will have to move their code to target the Bartik, Umami, Seven or Claro templates and libraries.

Data model changes

Release notes snippet

CommentFileSizeAuthor
#29 interdiff_27-29.txt3.52 KBmeenakshig
#29 3095713-29.patch5.9 KBmeenakshig
#27 interdiff_11-27.txt7.86 KBmeenakshig
#27 3095713-27.patch5.9 KBmeenakshig
#26 interdiff_11-26.txt5.06 KBmeenakshig
#26 3095713-26.patch0 bytesmeenakshig
#11 3095713-11.patch6.24 KBbnjmnm
#11 interdiff_9-11.txt422 bytesbnjmnm
#9 3095713-9.patch6.22 KBbnjmnm
#5 3095713-5.patch3.16 KBbnjmnm
#5 interdiff_2-5.txt4.26 KBbnjmnm
#2 3095713-2.patch2.55 KBbnjmnm

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Active » Needs review
StatusFileSize
new 3095713-2.patch2.55 KB

This patch adds a classy directory to the templates directory of Bartik, Seven, Claro and Umami.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented 
  1. +++ b/core/profiles/demo_umami/themes/umami/templates/classy/README.txt
@@ -0,0 +1,7 @@
+Classy is removed in Drupal 9 so themes extending it must now have their own

    s/Classy is removed in Drupal 9/Classy will be removed during Drupal 9

    themes extending it must now have their own
+copies of these templates.

    This might give false impression that this is the only way to solve this problem. Maybe we could just say that this change is being made in preparation for Classy to be removed during the Drupal 9 life cycle? 

  2. +++ b/core/profiles/demo_umami/themes/umami/templates/classy/README.txt
@@ -0,0 +1,7 @@
+If a template nested anywhere in this directory is changed, it should be moved
+outside of the Classy directory as it is now custom to the theme.

    I'm wondering if we should change changed to customized because we might want to copy bug fixes from Classy to these files in future 🤔

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new interdiff_2-5.txt4.26 KB
new 3095713-5.patch3.16 KB

All good points in #4, here's another revision.

Status: Needs review » Needs work

The last submitted patch, 5: 3095713-5.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Should we add folders for CSS files as well?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new 3095713-9.patch6.22 KB

Should we add folders for CSS files as well?
Yes, especially since thats an assumption I realize I've been making in some other works in progress. This patch adds them.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented 
+++ b/core/themes/bartik/templates/classy/README.txt
--- /dev/null
+++ b/core/themes/claro/classy/README.txt

+++ b/core/themes/claro/templates/classy/README.txt
--- /dev/null
+++ b/core/themes/seven/classy/README.txt

Why are these folders in the root, not in the CSS folder?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new interdiff_9-11.txt422 bytes
new 3095713-11.patch6.24 KB

Why are these folders in the root, not in the CSS folder?

M I S T A K E ☠︎

Fixed in patch.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

Awesome! Ship it 🚢

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Title: Create classy directory with README, in the templates directory for all themes subtheming Classy » Create classy directory with README, in the templates and css directories for all themes subtheming Classy
Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3095713-11.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs work » Reviewed & tested by the community

Unrelated test failure set this to NW, setting back to RTBC

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 8.9.x-dev » 9.0.x-dev
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 9.0.x-dev » 8.9.x-dev

Hmm, not sure if this should be backported to 8.9.x. Moving back just to make sure.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Related to #17, I'm unsure if we should backport issues decoupling Classy to 8.9.x because this could potentially cause disruption to themes extending Umami, Bartik, Seven or Claro. There should be no problem in making this change given that according to our policies, these themes are internal and shouldn't be extended. However, it seems potentially a bad trade-off to backport these to 8.9.x since the only benefit would be easier backporting of issues. This isn't all that valuable to us given that we've only made a handful of changes to Classy during the whole Drupal 8 life cycle.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs issue summary update, +Needs release manager review

I'm leaning toward 9.0.x only based on #19, but I think a bit more info would be helpful:

  • Can we update the IS to describe what usecases (including edgecase or "internal" usecases) might be disrupted by the change?
  • Are there any downsides to doing it in 9.0.x only WRT the continuous upgrade path? Is there any middle step we could put in 8.9.x?
  • I think it probably should have a CR. While it might disrupt internal architecture only, it's a systematic/pattern change for those internal subsystems rather than a single isolated change.

Thanks!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Are there any downsides to doing it in 9.0.x only WRT the continuous upgrade path? Is there any middle step we could put in 8.9.x?

Probably the most concerning downside is that for some themes it could be more difficult to support Drupal 8 and Drupal 9 simultaneously because of the differences in the major versions. Another downside is that it will be more difficult for us to backport issues.

A good middle step would be backporting copying all the templates to Drupal 8.9.x, but keeping the base theme setting as Classy. This means that pre-existing template extends and library overrides and extends should continue to work. At the moment I can't think of any downside in doing this middle step, and it should mitigate both of the concerns on not backporting this.

I think it probably should have a CR. While it might disrupt internal architecture only, it's a systematic/pattern change for those internal subsystems rather than a single isolated change.

I was thinking of this earlier. I'm wondering if we should make the CR part of this issue or as part of the follow-ups actually making the changes? Technically this issue is only about adding documentation so that the folder structure remains consistent in all themes. This won't have any effect on end-users.

xjm’s picture

xjm
she/her
Primary language English
commented

Thanks @lauriii. The BC layer for the 8.9.x backport sounds like a good idea -- let's get an 8.9.x patch as well so that we can review that and the 9.0.x patch together when planning how to commit this.

Regarding the CR, I think we should start one here that describes the initial change (including both the planned change for 9.0.x and the 8.9.x compatibility layer). Then, we can attach the same CR to the followups and add additional information to it as we commit the subsequent changes.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -Needs change record

I created a stub change record here: https://www.drupal.org/node/3103178. I didn't fill any specific examples yet since we don't know exactly how it will be yet.

I don't think we need a new patch here since this is just adding a bunch of README files. I added a comment to the parent issue to take this into account in the issues that are doing the actual moving templates over #3050389-12: [META] Remove dependency to Classy from core themes. What is important here is that we backport this to 8.9.x.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: -Needs release manager review

The CR looks good. I made a couple small tweaks to the text. And yeah, backporting this to 8.9.x makes sense based on #23 and the fact that it's just the directories (sorry, I hadn't actually read the patch yet before). 

  1. +++ b/core/profiles/demo_umami/themes/umami/css/classy/README.txt
@@ -0,0 +1,12 @@
+WHAT IS THIS DIRECTORY USED FOR?

    Couple small points:

    • Technically, this sentence ends with a preposition, which we are taught not to do in English classes in American schools. (Nonsense up with which I will not put!)

      I checked the Content style guide. It does not mention prepositions and specifies The Chicago Manual of Style, which allows prepositions at the end of a sentence, so this is fine and don't let anyone tell you otherwise. :P
       

    • That said, the word "used" is unnecessary, so I would omit it.

  2. +++ b/core/profiles/demo_umami/themes/umami/css/classy/README.txt
@@ -0,0 +1,12 @@
+Classy will be removed during the Drupal 9 lifecycle. To prepare for Classy's

    As written, this sounds like an alarming BC break. I would say "deprecated" instead of "removed".

  3. +++ b/core/profiles/demo_umami/themes/umami/css/classy/README.txt
@@ -0,0 +1,12 @@
+If a CSS file nested anywhere in this directory is customized, it should be

    In context, it's not clear what "customized" means. I assume it means "differs from the Classy version of the {CSS file, template}", so let's say that instead.

  4. +++ b/core/profiles/demo_umami/themes/umami/css/classy/README.txt
@@ -0,0 +1,12 @@
+moved outside of the classy directory as it is now custom to the theme.

    "Classy" should be capitalized here.

    That said, based on the previous point, maybe we should rewrite this whole sentence to:

    CSS files that differ from the Classy versions should not be placed in this directory or any subdirectory.

dww’s picture

dww
we/he/they
commented

Re: #24.4:

"Classy" should be capitalized here.

-1 to this. It's a directory named classy, not the proper noun for the theme, and case sensitive filesystems exist.

That said, +1 to your "that said" text, since it avoids the problem entirely. ;)

Otherwise, +1 to everything in #24.

+1 to the CR text. Looks clear to me. Makes me realize I'll have to do some work for this on a site-specific theme I built extending bartik. ;)

+1 to RTBC once #24 is addressed.

Thanks,
-Derek

meenakshig’s picture

meenakshig commented
Status: Needs work » Needs review
StatusFileSize
new 3095713-26.patch0 bytes
new interdiff_11-26.txt5.06 KB

Added the changes suggested in #24

meenakshig’s picture

meenakshig commented
StatusFileSize
new 3095713-27.patch5.9 KB
new interdiff_11-27.txt7.86 KB

Added the correct patch and the changes suggested in #24

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Thank you @Meenakshi.g!

+++ b/core/profiles/demo_umami/themes/umami/css/classy/README.txt
@@ -0,0 +1,11 @@
+CSS files that differ from the Classy versions should not be placed in this directory or any subdirectory.

Nit 🧐: This line is now over 80 characters per line: https://www.drupal.org/docs/develop/standards/coding-standards#linelength

meenakshig’s picture

meenakshig commented
StatusFileSize
new 3095713-29.patch5.9 KB
new interdiff_27-29.txt3.52 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

  • xjm committed 66aa8b0 on 9.0.x 
    Issue #3095713 by bnjmnm, Meenakshi.g, lauriii, xjm, dww: Create classy...

  • xjm committed 34bb006 on 8.9.x 
    Issue #3095713 by bnjmnm, Meenakshi.g, lauriii, xjm, dww: Create classy...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I've committed this to 9.0.x and 8.9.x, as well as attached and published the CR. (We can update it as needed as we do further development). Thanks everyone!

chi’s picture

chi commented

@xjm, that CR claims that Drupal core themes no longer extend Classy but that's not yet true. Shouldn't we have published it after #3050389: [META] Remove dependency to Classy from core themes?

Status: Fixed » Closed (fixed)

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