Problem/Motivation

The Classy base theme in Drupal 8 contains almost every core module template. These templates are being organized in a themer-centric way, rather than just mimicking the module organization scheme. While this will certainly make some aspects of theming more intuitive, this organization may also create a disconnect between the templates in the base-theme and their associated core templates in the modules directory. Further complicating things, the exact use of some template files is not immediately obvious and needs more explanation.

Proposed resolution

Every template in Classy should include a meaningful description of what it's used for, a reference back to the original template in its associated core module, and an example link to where its output can be seen.

Remaining tasks

  1. Agree on a standard format for this documentation in template files.
  2. Add it to all the files.

Comments

Cottser’s picture

Component: theme system » Classy theme
Issue tags: -theme, -system cleanup +theme system cleanup

Would this be just Classy? The issue summary makes it sound so, so updating the component.

jstoller’s picture

I think we need to start with Classy, but I expect some of this documentation will then need to be ported back to the module template files and probably to the other core themes.

davidhernandez’s picture

Component: Classy theme » documentation

I think if we add the example text to Classy we should really think about adding it to the core templates as well. They both have the same problem. Also, changing component to Documentation so docs team people are more likely to see it.

Cottser’s picture

+1!

jstoller’s picture

@davidhernandez: In principle I agree with you, but Classy (and other themes) have some requirements that core module templates do not. Most importantly, they need to include a reference back to the original core template. The core template, being the core template, doesn't require that reference. :-) In any case, that's why I say start with Classy. Then the Classy specific information can be removed and applied to the core templates.

davidhernandez’s picture

I'm only referring to the example usage text (where you can see this template in action.) That part is useful to both sets of templates. If it is useful for one, and we define a standard, there shouldn't be anything stopping us from using it for both since the text itself would be the same.

jstoller’s picture

Agreed. In fact, it should be ported to all the core themes, so Bartik and Seven can share the love.

jhodgdon’s picture

David Hernandez put a tell in IRC to ask me to comment on the parent issue; I'm assuming the comments are requested here?

On the parent issue, I see two options being proposed.

Comment #143:

{#
/**
 * @file
 * Two column template for the block add/edit form.
 *
 * Original template: core/themes/classy/templates/admin/block-list.html.twig
...

(adds the "original template" line)

And comment #144:

...
 *
 * @see /core/modules/block/templates/block-list.html.twig
 *
 * @ingroup themeable
 */

(instead adds @see near the end)

Here are my thoughts:

a) There's already a lot of duplicated information in theme templates... I have been meaning to bring this up for a while actually. What I mean is that for example if you're looking at the Classy version of block-list.html.twig, it has the same docs as the Core version of the same template. This seems ... tedious, silly, and hard to maintain, at best.

b) @see does not currently work with full file paths, so the proposal of using @see will probably not work well in practice. If you're on api.drupal.org, however, you'll already see links at the top of each template page to other templates with the same end file name. For instance, look at the top of:
https://api.drupal.org/api/drupal/core!themes!classy!templates!block!blo...
where it has links to the two versions, in core/themes/classy/templates/block, and core/modules/block/templates

c) Note that the existing .html.twig files, at least most of them, are in violation of our current docs standards, especially the first line:
https://www.drupal.org/node/1354#templates

d) The goals here are:
- Make sure themers have the information they need: variables available, what the base template is
- Make sure Core developers do not have a huge burden to maintain docs

I have some suggestions about how to resolve this, but need to run so I will add them in a separate comment.

jstoller’s picture

@jhodgdon: Given what I know about the ways templates are used and the people who use them, I'm inclined to believe some duplication of documentation is warranted here.

As the canonical copies of templates, the core module template files should each contain complete documentation about what they are and how they can be used. I expect everyone would agree with this.

I would argue that base themes—as the primary connection point for themers—should also contain this documentation. In core that means Classy. In addition, base theme templates should contain a pointer back to their original core module template. Yes, it will take a little more effort to maintain the documentation in two locations and, yes, you could just provide the pointer and make people refer back to the module template file for the documentation, but having the documentation in the template offers a significantly better themer experience making it worth the small upkeep cost, IMHO.

Sub-themes are a different story. Bartik and Seven are not intended to be used as base themes and aren't really even intended to be models for other custom themes. Given that, I could see limiting the documentation in their templates to a reference back to the Classy template being overridden, and a reference to the original module template.

jhodgdon’s picture

That makes sense to me, regarding the duplication of docs.

And thanks for reminding me I didn't get around to my additional comment -- time got away today.

So. given #9 and #8, what I'd like to suggest is:

-----
a) Templates in core/modules should be fixed so that their first lines comply with our current standards:
https://www.drupal.org/node/1354#templates
Basically they should look like this:

/**
 * @file
 * Displays a list of forums.
 *
 * Available variables:
(list of the variables)
 *
 * @see template_preprocess_forum_list()
[links to any other preprocess functions if necessary]
 *
 * @ingroup themeable
 */

The current first lines are more like "Default theme implementation for the administrative toolbar. " This is not horrible but isn't according to our current standards, which would make this "Displays the administrative toolbar.", which is shorter.

b) A Core base theme template that overrides a core template directly (same filename) would be the same, but it would have an additional line, so the top would look like:

/**
 * @file
 * Displays a list of forums.
 *
 * Overrides core/modules/foo/templates/foo.html.twig.
 *
 * Available variables:

In addition, the @themeable line would ***NOT*** be included, because that should only be on the main core implementation.

c) A Core non-base theme would have an abbreviated header:

/**
 * @file
 * Displays a list of forums.
 *
 * Overrides core/modules/foo/templates/foo.html.twig. See that file for complete
 * documentation.
 */

d) If a theme (base or non-base) has a specific override that uses theme suggestions, such as defining a block--foo.html.twig file for specific foo-types of blocks:

<code>
/**
 * @file
 * Displays a foo block.
 *
 * Theme suggestion for block.html.twig. See that file for complete documentation.
 */

And in this case, the suggestion file would hopefully be in the same directory as the main template.

-----

Thoughts? Let the bikeshedding begin!

davidhernandez’s picture

#10 looks pretty good to me. We keep the variables documentation, but remove the @themable and preprocess link, which might be confusing to people anyway.

Do we have leeway to add paths to the description. This is one thing we were looking to do, because finding a place where the template is actually used can be difficult for the more obscure ones. For example, I encountered this recently when working on the indentation template. It's description would be something like "Displays a set of indentation divs." but that doesn't tell you where you can see an example of the template in action. One of the only places to see it used is on the page to admin menu links, which took a little searching.

In the parent issue I had this line:

Example usage: Drag and drop table indentation (admin/structure/menu/manage/admin)

Could we do something like this, or work that concept into the description?

jhodgdon’s picture

The one-line description needs to stay under 80 characters, but of course you can add additional documentation. I didn't put that in the example... so how about this:

/**
 * @file
 * Displays a list of forums.
 *
 * Optional additional documentation, such as a path where this is used, or further
 * description of what it does.
 *
 * Available variables:
 --- (list of the variables) ---
 *
 * @see template_preprocess_forum_list()
---  [links to any other preprocess functions if necessary] ---
 *
 * @ingroup themeable
 */
davidhernandez’s picture

That works for me. Thanks, Jennifer! Any other thoughts?

I think we should wait until the Classy templates are re-organized, which should be soon, and then think about setting up a meta/sprint to redo all the template docs.

jhodgdon’s picture

No, I don't really have any other thoughts. The proposals in #10/#12 are basically "Follow our existing standards". We already have a precedent for "Document it once and provide links" for other stuff, so the suggestions there are just sort of a concrete template that can be followed easily. I don't think we need to have a formal "standards" discussion about it -- so yes let's turn this into a Meta when the reorg is done, and update the existing templates so they fit with these templates.

I suppose we should put these into the issue summary, after a couple of days to see if anyone else has ideas/discussion/comments?

Cottser’s picture

This is looking good to me, thank you all especially @jhodgdon :)

jstoller’s picture

@davidhernandez: I understand removing @ingroup themeable from the templates in Classy, but I wouldn't remove the preprocess function list. That is a useful reference for themers who are trying to figure out how this all works.

I still think the core module reference should be moved down below the file documentation and variable listing, along with the preprocess function list. All of those pointers to related stuff are less important than the other documentation, so they should go at the bottom, and its better if they're grouped together. So we'd end up with something like this:

In the core module...

/**
* @file
* Displays a thing.
*
* Optional (but strongly encouraged) additional documentation, 
* with more in-depth description of the thing and what it does.
*
* Example usage: The thing that does stuff at
*   admin/structure/menu/manage/admin.
*
* Available variables:
--- (list of the variables) ---
*
* @see template_preprocess_foo()
---  [links to any other preprocess functions if necessary] ---
*
* @ingroup themeable
*/

In Classy...

/**
* @file
* Displays a thing.
*
* Optional (but strongly encouraged) additional documentation, 
* with more in-depth description of the thing and what it does.
*
* Example usage: The thing that does stuff at
*   admin/structure/menu/manage/admin.
*
* Available variables:
--- (list of the variables) ---
*
* Overrides core/modules/foo/templates/foo.html.twig.
*
* @see template_preprocess_foo()
---  [links to any other preprocess functions if necessary] ---
*/

In Bartik/Seven...

/**
* @file
* Displays a thing.
*
* Overrides core/modules/foo/templates/foo.html.twig. 
* Overrides core/themes/classy/templates/misc/foo.html.twig. See that file
*    for complete documentation.
*/
jhodgdon’s picture

RE #16:

The only thing to remove from Classy is the @ingroup line. Nothing else. A line is added. See #10, which spells this out in item (b).

Also the formatting/indentation in #16 is not really useful. Keep in mind this documentation is parsed and displayed on api.drupal.org. Indenting lines within a paragraph doesn't do anything useful there. So this isn't going to be effective, and it goes against our general docs guidelines:

* Example usage: The thing that does stuff at
*   admin/structure/menu/manage/admin.

I also don't think we want to suggest either that every theme template needs example usage. Like for instance table.html.twig or block.html.twig -- we don't need to provide examples for where those are used.

So... I'm not a big fan of the proposal in #16. I think we should stick with #10/#12.

davidhernandez’s picture

Note that the code I posted was not intended to be two lines, if that is where that started. The line wrapped.

So I noticed that some of what we are discussing is already in use, just not often. For example, this example text is in the user template.

/**
 * @file
 * Default theme implementation to present all user data.
 *
 * This template is used when viewing a registered user's page,
 * e.g., example.com/user/123. 123 being the user's ID.
 *
Like for instance table.html.twig or block.html.twig -- we don't need to provide examples for where those are used.

Agreed.

RE #16 I don't think we should have two override comments. It's likely confusing. I would only add the direct parent override.

jhodgdon’s picture

Just as a general note, while I'm generally in favor of standards, the more we specify as standards, the less room there is for common sense. So, I don't think that we should say in our standards something like "You need to give an example of where the template is used", because we probably don't need it in all cases. I also don't think we need to say that a description is "strongly encouraged", because in a lot of cases we don't need that either (for example, block.html.twig -- how much description do you need beyond the one-liner that should tell you it outputs a block?).

So. Let's keep the examples simple.

Here's one more proposal that hopefully contains all the best ideas from the last few posts:

a) Core base template in a core/modules/foo/templates directory:

/**
 * @file
 * Displays a list of forums.
 *
 * Optional description, which may include something like:
 * Used on admin/foo/bar to display the baz.
 *
 * Available variables:
(list of the variables)
 *
 * @see template_preprocess_forum_list()
[links to any other preprocess functions if necessary]
 *
 * @ingroup themeable
 */

b) Override of a base template, when it's in a base theme like Classy:
This should completely copy the base theme template's documentation, and then:
- Omit the @ingroup themeable line at the end.
- Add a line just below the one-line summary that links you to the base theme. So it would look like this at the top:

/**
 * @file
 * Displays a list of forums.
 *
 * Overrides core/modules/forum/templates/forum-list.html.twig.
...

c) Override of a base template, when it's in a non-base theme:

/**
 * @file
 * Displays a list of forums.
 *
 * Overrides core/modules/forum/templates/forum-list.html.twig. See
 * the base template for complete documentation.
 */

d) Override of a template using theme suggestions, whether it's in Classy or another base theme:

/**
 * @file
 * Displays a foo block.
 *
 * Theme suggestion for block.html.twig, used when the block ID is 'foo'.
 * See the base template for complete documentation.
 */

The assumption here is that the base block.html.twig file is in the *same directory*, so whether this is in Classy or Bartik, we should not need to duplicate all the information in block.html.twig here.

Thoughts?

Cottser’s picture

I'm liking #19 overall, just want to mention that #19.d is not necessarily a safe assumption. For example Bartik overrides field--taxonomy-term-reference.html.twig but not field.html.twig.

And @jstoller/#16 I see where you're coming from with having the reference to the parent template at the bottom, but I think having it below the summary matches much closer with our preprocess standards which makes for some nice "symmetry" IMO: https://www.drupal.org/node/1354#themepreprocess

jhodgdon’s picture

Good point in #20 about the overrides... odd, I am pretty sure in D7 you could not override just a theme suggestion for a template without also overriding the base template. 99.9% sure actually -- I got bitten by this more than once in D7. So this is possible to do in d8?

If it is, then I guess maybe we should make (d) in #19 be something like:

After those few lines, if it's in a base theme and the base template is not also overridden in this theme (block.html.twig in this example, continue with the full documentation from the base template. Omit this in non-base themes and omit it if the base template is present in the same directory.

How's that for an idea?

davidhernandez’s picture

I am pretty sure in D7 you could not override just a theme suggestion for a template without also overriding the base template

That was true, but fixed in D8.

andypost’s picture

joelpittet’s picture

Issue tags: -rc deadline +rc eligible

It's documentation so it should be RC eligable.

jhodgdon’s picture

If we're agreed on #19 with the addition of #21, then we should probably update the issue summary.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.