Problem/Motivation

Phase 2 of the consensus banana is to move classes out of core templates and into a "classy" base theme which Bartik and Seven will extend from. This will change the markup (classes) of any Drupal 8 themes that do not have a base theme specified.

Proposed resolution

Add classy.info.yml to core and make an announcement so that contrib themes/base themes that currently do not have a base theme specified can immediately change their THEMENAME.info.yml to specify classy as their base theme. This will minimize the disruption when the currently-default core classes are later removed from the core templates.

Remaining tasks

Patch

User interface changes

n/a

API changes

Themes with no base theme specified will have some class/markup changes later.

Files: 
CommentFileSizeAuthor
#20 2329501.20.patch2.94 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,673 pass(es). View
#20 12-20-interdiff.txt1.3 KBalexpott
#12 interdiff-2329501-1-12.txt973 bytesmdrummond
#12 2329501-12-classy.patch1.4 KBmdrummond
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,186 pass(es), 15 fail(s), and 6 exception(s). View
#2 2329501-classy-info.png30.62 KBCottser

Comments

davidhernandez’s picture

Status: Active » Needs review
FileSize
463 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,199 pass(es). View

The ball is rolling.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
30.62 KB

Thanks @davidhernandez! Looks good, the only thing is I'm not sure if we need a screenshot added.

davidhernandez’s picture

Issue tags: +Needs change record

How about we just add a Druplicon for the screenshot? I don't think there is any value in a page screenshot like Stark has, as this is only intended to be used as a base. Other base themes, like Omega, use a logo for the screenshot.

tim.plunkett’s picture

With beta1 imminent, and no actual code ready for classy, I'm not sure why we'd add this info file.
It will add a theme to the admin/appearance page that does nothing, and has a link to a page that does not reference it at all.

Wouldn't it make more sense to add this once there is a chance that Classy will happen in time for release?

davidhernandez’s picture

We are moving along with the preprocess changes (#2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.) and we will need Classy to already be in place for the changes thereafter. The theme will not have any functional components beyond the template files that are moved there, and no CSS. It will only be a base. We actually want it to be in place for beta so that themers can start setting it as their base in preparation for the template moves. As templates get moved over, it will maintain functionality for them. If we wait until much later down the road, the template moves will cause greater problems for people.

I asked the core committers about this early this week. Alex said to go ahead and get it in now.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Let's get a draft change record written too.

davidhernandez’s picture

Status: Needs work » Needs review

Draft change record added. https://www.drupal.org/node/2337467

Cottser’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

CR looks good to me, thanks @davidhernandez.

It'd be nice if we could publish a short note about this on g.d.o/core as well to get it out there more.

alexpott’s picture

Assigned: Unassigned » Dries

Adding a new theme is one for Dries.

wheatpenny’s picture

+1 on the name choice. It explains what the purpose of the theme is.

mdrummond’s picture

Just a quick note on why we should add classy.info.yml now, rather than later.

Right now we are focused moving class selection logic from preprocess to templates. The next step is to move those templates into the Classy theme. At that time, the templates left in system/core will have the vast majority of their classes removed.

Currently all Drupal 8 contrib themes are based off of those system/core templates with the assumption that those classes exist in the templates. So if contrib themes continue to use system/core as their base, then the markup for those themes could change as those classes are removed, unless the theme overrides those templates.

The classy theme will have the same markup that the system/core templates previously did. So by creating classy.info.yml now, contrib themes (and Bartik and Seven) can set Classy as their base, and the markup they're assuming exists will remain the same as we move the class-filled templates into the Classy theme.

So I'm 1000% +1 on making this change now.

mdrummond’s picture

Title: Add classy.info.yml to core » Add classy.info.yml to core, set classy as base theme for Bartik and Seven
Status: Reviewed & tested by the community » Needs review
FileSize
1.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,186 pass(es), 15 fail(s), and 6 exception(s). View
973 bytes

I think it's worth setting classy as the base theme for Bartik and Seven in this same issue. It has to be done anyhow, and that way this serves as a nice clean example for how contrib themes can do the same thing.

Cottser’s picture

Title: Add classy.info.yml to core, set classy as base theme for Bartik and Seven » Add classy.info.yml to core, set Classy as base theme for Bartik and Seven
Status: Needs review » Reviewed & tested by the community

I agree. For a seamless testing/upgrading experience I recommend:

drush en classy

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2329501-12-classy.patch, failed testing.

tim.plunkett’s picture

This seems to imply that theme initialization doesn't really accommodate base themes.

mdrummond’s picture

I wonder if #2232605: Themes cannot be uninstalled might help with this. Gets rid of theme enabled/disabled status and moves to just theme installed/uninstalled.

mdrummond’s picture

Just a note that when I did a clean install with this patch applied, I didn't get any errors, and classy showed up as being enabled.

mdrummond’s picture

I wonder if in ConfigImportUITest we need to explicitly enable classy since Bartik depends on it, and some of the tests revolve around enabling and disabling Bartik?

tim.plunkett’s picture

Either base themes should not need to be installed for their subthemes to work (like D7), or installing a subtheme should install the base theme. I disagree with changing the test coverage to work around this apparent bug.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
2.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,673 pass(es). View

Base themes do need to be installed the configuration created in staging by ConfigImportUiTest is incorrect since in order for bartik to be enabled with this change classy must be to. The other test failure is because classy appears before stark on the theme appearance page.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @alexpott!

jstoller’s picture

So, back in Austin when we came up with this crazy scheme, "Classy" was being used as a placeholder for some yet-to-be-determined theme name. Now it seems like Classy has made the jump from placeholder name to actual name, without any real discussion. I propose we take a step back and have that discussion before we move too much further.

Personally I think "Classy" is hilarious, but I'm an insider. From a usability standpoint we may want to do something more generic and descriptive. Like "Base," or "Core," or "Core base."

Should we spin off a separate issue for this?

alexpott’s picture

@jstoller this is precisely the issue to discuss this. I have no opinion on this personally - naming stuff should be easy but it always turns out to be hard. However, I would say that calling a base theme "base" or "core" or "core base" is actually less descriptive.

davidhernandez’s picture

Several of us discussed the name during a few of the Twig conference calls and in IRC, and we were happy with the name choice, and not just in an insider joke way. (Which I agree can sometimes get annoying.) It is short, descriptive, and made sense given it's contrast to Stark. Stark/core being devoid of classes, and Classy being enriched with classes. No other name we came up with was any more fitting, just different.

pwolanin’s picture

@jstoller - it's a totally reasonable name for what it is. Can we say "Seven" is better? The only point in stepping back and discussing is if you want to derail this issue and block this change. If you support it at the technical level, let's get it in without an endless bikeshed about naming a YAML file.

crowdcg’s picture

+1 for this change. I was in the Banana discussion in Austin too and subsequent Twig calls and IRC chats. No other name seemed to work well and "Classy" is short and descriptive.

mortendk’s picture

ooooh this could be the epic bikeshed ;)

im perfectly fine with the name classy cause it actually explains what it does + have a meaning in the history of drupal theming.

About naming themes i was responsible for getting "blue cheese" in as the d.o name way back, as a joke on the old name "Blue beach" (and it makes me chuckle a little bit everytime people go wtf blue cheese)

Personally I love that we have fun with our naming themes + that theres a history behind it, and theres so many fun little things to say about finally having a classy basetheme in Drupal (drumroll)
To be a gentlename I will also abstain from trying to bikeshed thise by demaning the thene to be called "Banana" ;)

Now Dries ... get the sushi outta ya mouth and lets gogoggogogog :)

mdrummond’s picture

+1 to Classy.

We thought of other names, but really Classy is a good name, in that it describes exactly what it does, like Stark. I really don't think there's a need to bikeshed this. Let's get this in so contrib themes can start pointing to this.

jstoller’s picture

My intention was certainly not to bikeshed or derail this issue. I just thought it should be discussed, as originally intended. I was not privy to whatever discussions happened on IRC and saw no mention of them here until now.

Trying to look at this from the perspective of a new-to-Drupal themer, I saw this theme as "the standard Core base-theme," as opposed to "the theme with all the classes," in which case Classy is somewhat less descriptive. That said, there are far worse names for this thing and I certainly am not going to stand in the way of progress. If it's been discussed in a meaningful way and this is the consensus, then long live Classy!

alexpott’s picture

Issue tags: +beta target

D8 themes need to depend on this base theme to get all of core's classes - this is surely nice to have as early as possible in the beta.

xjm’s picture

Priority: Normal » Major
mdrummond’s picture

#30: Yes, that's why we wanted to get this in now, even before beta if possible. That way we can still move templates over to Classy afterwards, but if contrib themes are already pointing to Classy as their base, everything will work well and not break.

#29: Just to provide some extra clarification. It is not a guarantee that all contrib themes will point to Classy as their base theme. If they don't set a base (outside of subthemes pointing at a starter theme), then they'll get the system/Stark version of core markup, which will have all but the most essential classes removed. So the contrib themes that want a good class-based starting point for theming set Classy as base. Those that want nothing, don't. Due to that, I think the name Classy is pretty descriptive of what you get by setting that as the base theme.

LewisNyman’s picture

Whoop! +1

Classy is classy.

LewisNyman’s picture

Would it make sense to have a flag for theme that are only intended to be used as base themes? Then we could prevent them showing up in the UI.

jstoller’s picture

@LewisNyman, it seems like we should give people the option to see what a site looks like in a raw base theme, so I would still want them to show up in the UI. However, I think a flag for base themes is certainly a good idea, and we could segment them off in their own section of the UI. I know when I first started playing with base themes I was a little confused about whether I needed to activate Zen or not, in order for my subtheme to work. Moving base themes into their own box could help with that, as well as better direct users to information about the proper use of base themes.

joelpittet’s picture

And some base themes (Omega 4) recommend you enabled it because it does some hooks that don't fire when it's not enabled.

davidhernandez’s picture

@jstoller, yes, that is a good point about seeing the markup. You can enable Stark to see core's eventual simplified markup. Enabling Classy would be good to see how its markup and classes render.

alexpott’s picture

@joelpittet it is not possible for a base theme to be uninstalled and themes that depend on it be installed - it was in d7 but not in d8

joelpittet’s picture

@alexpott ah thanks, well that keeps things less ambiguous

mdrummond’s picture

I think separating out base themes might be nice, although that's a new feature not required to get this in.

We have a fair amount of discussion of this at RTBC. Can we make this Super RTBC? :) I think this is ready to go in.

sqndr’s picture

Classy++ !

  • Dries committed 41fce63 on 8.0.x
    Issue #2329501 by alexpott, mdrummond, davidhernandez | Cottser: Add...
Dries’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed to 8.x (from the Core conversations Q&A). Thank you! :)

Status: Fixed » Closed (fixed)

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

mirom’s picture

Issue from updating from beta1 to beta2 - in beta1 Classy was not enabled, but beta2 requires it, otherwise it throws
PHP Fatal error: Call to a member function getPath() on a non-object in /core/lib/Drupal/Core/Theme/ThemeInitialization.php on line 163
Please, modify configuration to automatically enable Classy.