A sandbox have been created for POC https://www.drupal.org/sandbox/cottser/2297653

Problem/Motivation

Drupal themers are a diverse bunch of folks. After completing a survey of themers, we can divide them into 2 camps:

  1. Want sensible markup with sensible default classes so they can apply CSS with minimal markup overrides. Roughly 2/3 of themers fall in this camp. We will refer to them as the "sensible 2/3".
  2. Want minimal markup with no default classes so they can add their own classes and override the markup when necessary. Roughly 1/3 of themers fall in this camp. We will refer to them as the "clean 1/3".

The goals and requirements of these 2 groups is difficult to reconcile in a single set of "default templates" that are provided by Drupal core. Currently, we solve for the 2/3 camp and when faced with a tough decision, we should continue to side with that camp. However, it would be very nice if we could cater to both groups.

Additional tangential problems: (that will become relevant when discussing the solution)
All Drupal themers have difficulty in finding the template files in the various places in /core/modules. A previous suggestion was that all template files should be moved to a single directory in core, but that solution had bad DX for module developers.

In Drupal 6/7, we mistakenly advised that all logic should go into preprocess functions and not in template files. This was partly because PHP code in templates is just fugly, but also because we didn't understand that it is perfectly fine to put all design decisions (including "design logic") together into the template file (e.g. the first item in this list has a special design, so we treat it differently.) Due to that mis-step, many themers (across both camps) have difficulty altering the classes added by preprocess functions in order to implement their design.

Proposed resolution

brief tl;dr description

Create a new core theme (code name "classy") that contains a copy of the all current core template files; this is for the "sensible 2/3" camp. And then modify all of the core/modules template files to contain minimal classes (only those needed for functionality); this would be for the "clean 1/3" camp. To ensure that Seven and Bartik continue to function properly, they should use "classy" as their base theme.

detailed plan

  1. Move classes out of the preprocess functions into the top of the corresponding twig files. Note: we will not be removing the classes preprocess variables, so modules (like RDF) will still be able to add classes from preprocess if they really need to.
  2. Make a "classy" base theme.
    1. Copy all twig files into the new theme.
    2. Core themes (except Stark) use "classy" for base theme.
    3. Remove classes (that are not necessary for module functionality) from twig templates in core/modules.
    4. Any CSS that is orphaned by the removal of classes will need to be moved to "classy". (Note: Yes, CSS files can be attached by "classy" via preprocess and a fugly hack until [ # ???? need issue number from Crell ] lands.)
  3. Simplify the markup in twig templates in core/modules.

All of this will need to be done by Drupal 8.0-rc1 when we hit "markup freeze". But if we fail to complete the full list, the ones already completed can stand on their own (i.e. we don't have to roll anything back.)

What it solves

  • For the first time, the clean 1/3 get catered to (with core/modules), while, at the same time, the sensible 2/3 are catered to (with "classy".)
  • Easy to investigate and view both sets of markup. Use "classy" to see its markup and use Stark to see core/modules.
  • We finally have a proper base theme in core. (NO, Stark was never a base theme.)
  • The "template findability" problem: having the "classy" theme contain a copy of all core/modules templates would give a centralized location for discovery by the sensible 2/3. The docblocks of the "classy" templates would also point at the location of the core/modules original which would help the clean 1/3.
  • It's hard to modify classes that are created in a preprocess function. Modifying Twig logic is much easier.
  • Even if contrib modules only build "classy" versions of their default templates, having the classes specified in the twig files will make it much easier for the clean 1/3 to clean up those contrib twig files. And vice versa if the contrib module only provides clean 1/3 versions of their templates.
  • It makes it easier to implement a theme using the "cool new framework" of the moment. Want a Bootstrap theme? Or an announced-just-this-week Web Starter Kit theme? Either fork "classy" and start tweaking classes or copy twig files from core/modules and start adding classes, which ever fits better with your goals.

Known Issues

  • We would have 2 themes in core that basically have no design, Stark and "classy". JohnAlbin would argue that they serve 2 different audiences and front-end developers would understand the dichotomy. Even with "classy" in core, Stark would continue to be a window into what the core/modules provide (which has always been its purpose.) However, if this becomes a show-stopper, "classy" is better to have in core than Stark.
  • We would have 2 different versions of every template in core. It does suck a little, but we already have that issue with some template files. For templates in Bartik and Seven, we are already having to remember to edit both the core/themes one and the core/modules one. In fact, we often forget and end up with regressions in core themes. If we add classy, it would actually make it harder to forget; since all templates have a dupe in core/themes, it would become a habit to look in both places when making a change to a twig file.
  • Because core/modules and core/themes/classy have different purposes, it will be harder for back-end developers to figure out how to edit any template file in core. Yes, agreed. Which is why a front-end developer should be consulted when editing a template file. Communication, ftw!

Remaining tasks

@see #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.
@see [meta] phase 2 @todo

User interface changes

New "classy" base theme on the Appearances page.

API changes

Little stuff. Like all the markup.

Files: 
CommentFileSizeAuthor
#2 consensus-banana.jpg40.92 KBJohnAlbin

Comments

JohnAlbin’s picture

Issue summary:View changes
JohnAlbin’s picture

Issue tags:+Twig, +theme system cleanup
StatusFileSize
new40.92 KB

BTW, this issue was a group effort.

Members of the "consensus banana" discusstion at Drupalcon Austin

mortendk’s picture

As a sidenote the plan for the class name inside of the templates are also addressed here https://www.drupal.org/node/2254153

jenlampton’s picture

Issue summary:View changes
jenlampton’s picture

Issue summary:View changes
JohnAlbin’s picture

Issue summary:View changes
JohnAlbin’s picture

Issue summary:View changes
JohnAlbin’s picture

Issue summary:View changes

Note: Front-end devs like to joke around with each other. At various times, we've tongue-in-cheek referred to the other group as "world class lovers" or "no class themers", etc.

The sensible 2/3 and clean 1/3 monikers are ones I just came up with. My hope is they are informative, minimal and un-inflammatory. :-)

Crell’s picture

Speaking from a core dev/module dev perspective, I support this banana.

JohnAlbin’s picture

LewisNyman’s picture

Issue summary:View changes
LewisNyman’s picture

Issue summary:View changes

I've compiled a list of all the theme functions where classes are added in a preprocess function. Feeling sad now.

Cottser’s picture

I would suggest that we come up with (some) sensible groupings rather than creating an issue for each template/theme function. For example, file, group, aggregator can all be grouped I think. As long as the patch size is reasonable it will be easier to deal with than 3-4 smaller issues. However we learned in the Twig conversions that trying to tackle all of form.inc, system.module, etc. is not the way to go. We got way more velocity once we split those out.

mdrummond’s picture

After a number of fun discussions with Morten over classes vs no classes, I am all for this proposal. We'd discussed something along these lines, and this looks like a great way to go, as everybody wins.

Putting this in place will mean a lot less contention over the dream markup issues.

Looking at that long list of preprocessor functions, I am concerned about whether we can realistically get this done. When we have tried to move classes into the templates in one or two issues we ran into some pretty thorny issues.

There are also still a number of conversions of theme functions to twig templates remaining. So all of those need to get done, all the preprocessor classes to template conversions would need to get done, plus tackling dream markup. It's a long list and a relatively small set of people who've been working on these issues.

Setting up the classy theme itself shouldn't be too bad. I suspect there will be another set of discussions about what actually goes into the system templates once classy exists.

That said, if we actually accomplish all of this, it would definitely rock.

jstoller’s picture

Issue summary:View changes
LewisNyman’s picture

Issue tags:+frontend
mortendk’s picture

Issue summary:View changes
Issue tags:+banana
nod_’s picture

1 issue per function is going to be hell. I'd say make one issue per module at most and spin off follow-up for problematic function when needed. Otherwise we'll still be hearing about this in 2015.

Cottser’s picture

Issue summary:View changes

@nod_, yep. Small tweak to the issue summary for now.

davidhernandez’s picture

Is there clarity on the approach? Are we talking about basically applying the patch from https://www.drupal.org/node/2254153#comment-8925239 , with Mark's changes from https://www.drupal.org/node/2254153#comment-8936375 , then repeat for all the preprocess functions in core? After that, add "classy" with copies of all core twig templates and added classes. Then, discuss what classes to strip completely out of the core templates so they won't get inherited by the new Stark?

LewisNyman’s picture

Yep, that's how I understand it. I don't understand if we should be working on this in core issues or in the sandbox now?

crowdcg’s picture

@davidhernandez was waiting on joelpittet to update the addClass patch. Then it's a matter of creating sub issues. wheatpenny and I suggested not doing more proof if concept work in the sandbox and just getting started on the subissues when ready.

Cottser’s picture

I think the sandbox is where we should be exploring things like "how will template inheritance work with this" so that if possible we can avoid duplicating entire templates where the only difference is what classes are added. It'll be easier to test that in a sandbox than applying a ton of patches to core to test. As @crowdcg said we should not be trying to do all the conversions in the sandbox, that can happen in the core queues once #2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names. is in.

joelpittet’s picture

@crowdcg Oh dang didn't know you were waiting on me, I'll do that up tonight. Sorry!

crowdcg’s picture

Now that #2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names. is in we should now be able to open the sub issues per module. I think it would be great if we took a quick look at #2254153: Move node classes out of preprocess and into templates, added the process to the issue summary and used it as a template for the others.

Thanks again to all those who worked on #2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names.!
@joelpittet, @davidhernandez, @cilefen, @mdrummond, @mortendk

davidhernandez’s picture

The change record for #2285451 has instructions for using the new methods. https://www.drupal.org/node/2315471

edit: ok, the filter doesn't work for change records.

davidhernandez’s picture

davidhernandez’s picture

davidhernandez’s picture

joelpittet’s picture

Issue summary:View changes

Removed the list from this Issue Summary and added link to class to template Phase 1 child meta.
#2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.

xjm’s picture

Issue tags:+rc deadline

Per summary, as well as convoluted discussion with @alexpott. ;)

davidhernandez’s picture

I started an issue to discuss phase 2. Classy is not in yet (hopefully, in before the Friday Amsterdam sprint,) but we can start discussing.

#2348543: [meta] Consensus Banana Phase 2, transition templates to the Classy theme

davidhernandez’s picture

Cottser’s picture

Woo! Just made a small tweak there to make sure it's mentioned specifically that Bartik and Seven inherit from Classy.

davidhernandez’s picture

Phase 2 issues have all been created.

xjm’s picture

Priority:Normal» Major