Problem/motivation

Following up on the agreed upon theme structure for Drupal8, stark & bartik should be developed separately.

Stark is bloated with unnecessary markup & css that makes it hard for new themes to create the markup & css structure thats needed. cleaning up stark will not mean that we remove the markup structure & classes for bartik.

Starks maintenance-page.html.twig should be html5 & follow the same structure as page.html.twig.

This patch dosn't try to solve any of the (many) issues with Bartik - bartik will be addressed when Stark is cleaned up.

This is for moving stark forward, not bartik.

Proposed solution

* Menus menu attributes is removed from stark and added to bartik's template.php
* #id's are removed if they serve no purpose
* layout css follows the css layout rules (used of l-foobar classes)
* maintenance-page.html.twig as html5 & follows the same structure as stark
<div role=main> got the appropriate tag <main role=main>
messages wrapper div is removed as its not necessary
* clearfix removed from the markup for main & added in the css main:after to make sure we dont force that solution into the core markup
* removed the styling of <li> as its not nessesary
* removed wrapper div & p around site-name link

Issues

By removing starks inline class the menu links do look different:
The show the list style & isnt inline display'ed
If stark is to show what comes out of Drupal by default it makes perfectly sense that the li is "stark naked" as its not a designed theme.

Related issues

#1980004: [meta] Creating Dream Markup

#2041793: install-page.html.twig markup and CSS

CommentFileSizeAuthor
#71 twig-maint-page-head-tag-restored.patch405 byteseatings
#64 page-stark-2011578-64.patch20.28 KBsoulston
#59 2011578-59.patch20.29 KBstar-szr
#59 interdiff.txt1.26 KBstar-szr
#58 page-stark-2011578-donedonedone.diff20.38 KBmortendk
#56 page-stark-post-contextual-on.png199.42 KBmortendk
#56 page-stark-post.png177.54 KBmortendk
#56 page-stark-pre-contextual-on.png200.31 KBmortendk
#56 page-stark-pre.png179.02 KBmortendk
#56 page-stark-2011578-sitename-clearfix.diff20.38 KBmortendk
#54 page-seven-prepatch.png164.17 KBmortendk
#54 page-seven-postpatch.png130.6 KBmortendk
#54 page-stark-postpatch.png203.56 KBmortendk
#54 page-stark-prepatch.png204.52 KBmortendk
#54 page-bartik-postpatch.png205.78 KBmortendk
#54 page-overlay-postpatch .png533.59 KBmortendk
#54 page-bartik-prepatch.png177.71 KBmortendk
#54 page-overlay-prepatch .png508.74 KBmortendk
#54 page-stark-2011578-donedone.diff20.06 KBmortendk
#51 page-stark-2011578-onlypage2.diff20.06 KBmortendk
#49 page-stark-2011578-onlypage.diff20.06 KBmortendk
#45 page-stark-2011578-nobody.diff20.57 KBmortendk
#36 page-stark-2011578-notodo.diff20.81 KBmortendk
#32 page-stark-2011578-3.diff21.14 KBmortendk
#30 page-stark-2011578.diff18.48 KBmortendk
#20 2011578-20.patch20.78 KBtlattimore
#20 interdiff.txt5.01 KBtlattimore
#16 interdiff.txt2.93 KBtlattimore
#16 2011578-16.patch15.97 KBtlattimore
#10 2011578-page_and_maintainence3.diff13.86 KBmortendk
#5 2011578-page_and_maintainence.diff16.96 KBmortendk
page+maintainence-page.diff14.06 KBmortendk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

This looks good, I was wondering if we should rename the page class to something more descriptive and inline with our CSS standards? I was thinking .l-container

mortendk’s picture

yup sounds like a better idea the class name i added in isnt optimal (yet)

we do need to have a good talk about what were gonna do with the menu variables and other hardcoded elements in d7 many of theres: menu, highlight etc was also avalaible as blocks that leed to a high degree of confusion.

LewisNyman’s picture

Yes let's do that, some of these variables are dependant on #1053648: Convert site elements (site name, slogan, site logo) into blocks.

The ones that are already available as blocks should be removed from the page.tpl.php and placed in a new or appropriate region. It's a complete WTF for new users.

Do we need separate issue for this? I don't think so, we should be affecting the output of Bartik and Stark with this patch.

mortendk’s picture

nopes lets keep the page(s).html.twig here else its gonna be a nightmare
the menus are about to go as well (or have been gone since d6 afaik) but have just been floating around #1869476: Convert global menus (primary links, secondary links) into blocks

mortendk’s picture

Status: Active » Needs review
FileSize
16.96 KB

updated the class names & a little bit more cleanup

if we still have regions they should endup with a l-region_$fooregionname naming instead of the "region region-$foo" we have now
so that would kill the l-sidebar-first & l-sidebar-first div wrappers

star-szr’s picture

Issue tags: +dreammarkup

Tagging.

star-szr’s picture

Title: markup for page.html.twig + maintanence-page.html.twig » Markup for page.html.twig + maintenance-page.html.twig
tlattimore’s picture

This is definitely looking much better! Will be really nice if we can get the region stuff figured out and get rid of these @todo's.

Status: Needs review » Needs work

The last submitted patch, 2011578-page_and_maintainence.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
13.86 KB

@tlattimore rerolled it removed the region changes - so we can move forward the regions is gonna be its own issue

mortendk’s picture

Title: Markup for page.html.twig + maintenance-page.html.twig » Markup for Stark's page.html.twig + maintenance-page.html.twig
mortendk’s picture

Issue tags: +Stark, +Twig

added tag for stark + twig

mortendk’s picture

Issue summary: View changes

better description

Status: Needs review » Needs work

The last submitted patch, 2011578-page_and_maintainence3.diff, failed testing.

tlattimore’s picture

Assigned: Unassigned » tlattimore

I will look into what is causing this to fail so many tests.

mortendk’s picture

yup looks like my ignorence towards test is now my nemesis

tlattimore’s picture

FileSize
15.97 KB
2.93 KB

Here is a patch to correct some of the errors reported for #10. Just changing some xpath()'s and correcting some variables in maintenance-page.html.twig that were causing the exceptions to be thrown.

tlattimore’s picture

Assigned: tlattimore » Unassigned
Status: Needs work » Needs review

Tagging for review. The patch in #16 still has lots of work to be done, but would be nice to see some test results with out all those PHP exceptions being thrown at least.

Status: Needs review » Needs work

The last submitted patch, 2011578-16.patch, failed testing.

tlattimore’s picture

I've gotten all but one of these test errors resolved I believe. Hopefully will get the other one finished in the next couple days and submit an updated patch.

tlattimore’s picture

FileSize
5.01 KB
20.78 KB

Here's is an updated patch that should resolve the remaining test failures.

tlattimore’s picture

Status: Needs work » Needs review

go ahead test bot.

mgifford’s picture

Why are you replacing ids with classes? I'm not sure what additional flexibility it gives you, but feels like a bit of a loss as far as structure goes.

LewisNyman’s picture

It's part of our new CSS standards:

Avoid using the id selector in CSS. There is no general benefit to using the 'id' attribute as a CSS hook, and it has serious downsides in the form of increased selector specificity.

mgifford’s picture

The avoidance of ID selectors I think goes back to http://smacss.com/book/type-layout

I'm wondering what if any evaluation is being done though to address when they are useful elements? From that SMACSS page:
"To be clear, using ID attributes in your HTML can be a good thing and in some cases, absolutely necessary. For example, they provide efficient hooks for JavaScript. For CSS, however, ID selectors aren’t necessary as the performance difference between ID and class selectors is nearly non-existent and can make styling more complicated due to increasing specificity."

This looks like a great initiative though, thanks for the link. Just the call to avoid using ID selectors is different than don't use ID selectors. When do we know if they are being leveraged or not?

mortendk’s picture

first off the template's here is ment for stark as the title say and the basic idea of stark is that we wanna make that as clean as possible (as outlines here: https://drupal.org/node/2008464#principles) - that alone is a good reason ;) btw more on stark & bartik: https://drupal.org/node/1854344

But just to make it clear why ID's are such a bad practice - The avaoidence of ID's goes back to when css started to really take off & frontend developers began to learn the trouble it had by having a selected that had such an high specificity. for a good run down look here http://coding.smashingmagazine.com/2007/07/27/css-specificity-things-you...

Using id's for selectors like a sidebar's as a base is simply bad practice, an id should only be used if there's a "real reason" for it, simply because its to hard to overwrite in the css, your gonna end up with lock down css thats less flexible by using ID's instead of classes - a real reason for an ID would be an anchor tag fx. else theres absolutely no reason to use it.

So it is actually a call NOT to use ID's - simply to stop the bad practice, unless theres an actual reason for it (kinda like when to use !important ;) )

mgifford’s picture

I've been trying to explore this, as I frankly haven't come across ID's as a problem before. You've definitely done a good job raising awareness as to why they have caused a problem for themers.

That being said there are solid reasons to use ID's for:

  • form elements to relate labels & inputs as well as fieldsets
  • other WAI-ARIA elements which actually add semantics
  • tables to provide header/cell relationships
  • javascript elements (if required)

Simply getting rid of all ID's from Core isn't an option, but that's what you are arguing for.

Some other related links I found interesting when digging into this.

Thread with references to WebAIM & changing thoughts of Jeffrey Zeldman:
http://www.impressivewebs.com/ids-not-make-documents-semantic/

I like how they talk about ARIA here:
http://oli.jp/2011/ids/

mortendk’s picture

Getting rid of all id's ? huh where do i write that - "So it is actually a call NOT to use ID's - simply to stop the bad practice, unless theres an actual reason for it (kinda like when to use !important ;) )"
As you point out theres a real reason for this when we work with a form or a table - But thats not what this issue is about this is about page.tpl + the maintainence-page.twig for Stark.

if you want to keep the ID's in the sidebars here, well i cant se any reasons for that - we are not talking about places where an ID make sense & getting the markup up to modern standards:

Styling with IDs and classes are one of the first things we learned about CSS, so it can be challenging to hear “don’t use IDs in CSS selectors”. However this industry is always changing.

http://oli.jp/2011/ids/#old-skool

This is not the place for a discussion about tables or forms this is only about the markup for stark page + maintaince page. I would suggest that we take em one after another or that you take that up over in the meta issue
https://drupal.org/node/2008464#principles

mgifford’s picture

Regarding "Getting rid of all id's" - totally my sloppy reading/responding. Early in the comment that's what you seemed to be suggesting, but that was definitely clarified by the end. I should have edited my comment more clearly.

I can't make a good argument (yet) for the sidebars, so until I'm convinced I won't be arguing for them....

Thanks for explaining this to me.

And ya, I'll take the discussion about forms/tables to the principles page and leave this to:

stark page + maintenance page.

So, way back in #20 @tlattimore produced a patch that the bots liked. What's the process to get this new clean code approved and brought into Core? Testing & screenshots. What's the best way to help move this ahead?

mortendk’s picture

This code is for stark - so its clean mean markup + make sure that we dont break bartik.

mortendk’s picture

FileSize
18.48 KB

wonder if bots like this but new interesting issues poped up:

{{ messages }} is still printed even if its empty prints out metadata http://pastebin.com/b2FRPd6p

Status: Needs review » Needs work

The last submitted patch, page-stark-2011578.diff, failed testing.

mortendk’s picture

FileSize
21.14 KB

id="messages" remove from the test

mortendk’s picture

Status: Needs work » Needs review
mortendk’s picture

Issue summary: View changes

reworded

mortendk’s picture

Issue summary: View changes

cleaned up description

mortendk’s picture

Issue summary: View changes

clearified the text

drupalninja99’s picture

@morten I did a cursory review of #32 and it looked good to me. I am not qualified though to give this a RTBC go-ahead =).

Brandonian’s picture

Status: Needs review » Needs work

Markup looks good, meshes with the new CSS/markup standards. Patch needs to be redone to remove various @todo's within the patch.

mortendk’s picture

Status: Needs work » Needs review
FileSize
20.81 KB

removed todo's reroll

Brandonian’s picture

New patch looks good. Assuming testbot likes it as well, RTBC.

sjbassett’s picture

Adding NYCcamp2013 tag

eatings’s picture

A couple points of inconsistency, such as in install-page.html.twig:

compare

{% if title %}<h1 class="title" id="page-title">{{ title }}</h1>{% endif %}

with

<div id="content" class="column">

ID's should come first, then classes, yes?

mortendk’s picture

The install page is another issue that i think we have to adress on its own - were trying to keep the patches small instead of ending up in a mega patch(tm)

ID's shall only be there if theres a really really really good reason for it & for h1 tag theres absolutely no reason for .title or even worse #page-title *gasp*

update: #2041793: install-page.html.twig markup and CSS is created hence its outta scope for this patch

eatings’s picture

Status: Needs review » Reviewed & tested by the community

Apart from the new issue in #40, this looks good.

edit: just waiting for testbot to clear. Otherwise, looks RTBC to me.

LewisNyman’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/templates/maintenance-page.html.twigundefined
@@ -12,80 +12,73 @@
+      <div class="l-messages">

I'm not sure why we're using a layout-only class for messages here. Can someone fill me in? I can't find any CSS that styles #messages or .l-messages in system or stark.

+++ b/core/themes/stark/css/layout.cssundefined
@@ -9,129 +9,131 @@
+  body.two-sidebars .l-sidebar-second {

Do we need the body declaration in the selector?

+++ b/core/themes/stark/css/layout.cssundefined
@@ -9,129 +9,131 @@
-  #sidebar-first {
+  .l-sidebar-first {
...
-  #sidebar-second {
+  .l-sidebar-second {

We could use a common class here like l-sidebar to cut down on repetitive CSS. But also see:

+++ b/core/modules/system/templates/page.html.twigundefined
@@ -110,40 +101,58 @@
+    <div class="l-content">
...
+      <aside class="l-sidebar-first" role="complementary">
...
+      <aside class="l-sidebar-second" role="complementary">

I'm not sure if l-content and sidebar makes sense here. Should they be more generic? If I wanted to add a background colour to either of these columns I wouldn't naturally think to add another class, but I would have to if I wanted to conform to our CSS standards.

What about something like l-column or l-major/l-minor?

mortendk’s picture

l-messages:
changed the #messages placeholder to .l-messages as Drupal atm have to have these hardcoded ind ( that should change later)
its a a placeholder & it have no visual styles, just like l-sidebarleft etc.

.l-sidebar-first/second
The sidebars are different so why not keep em different ;)

first - last ... bikeshed
Im not sure this patch is here to bikeshed naming of sidebars classes & how we think our classnames (or if they are even sidebars - they can be whatever)
Honestly i think thats another issue & putting that in as a blocker here, is imho bikesheding instead of progressing

If we need a discussion about nameschemes . that should imho be in another issue. remeber the 80% rule - we dont build for everything, thats a drupalism that have kept us from progress

LewisNyman’s picture

That's fine with me, we can have class naming discussions in a follow up

mortendk’s picture

Status: Needs work » Needs review
FileSize
20.57 KB

- removed the body.foo in the css
- removed the .l-message its an leftover from old times & only serve as a wrapper for the {{ messages }} no reason for this to still live
changes the <div role="main"> to the appropiate <main role="main> tag instead

please review (& testbot show me your magic)

mortendk’s picture

Issue summary: View changes

cleaned up a bit more

Brandonian’s picture

Looks good to me, and bot likes it. Marking as RTBC.

Brandonian’s picture

Status: Needs review » Reviewed & tested by the community

Actually marking RTBC.

star-szr’s picture

Issue summary: View changes

added info about main tag + removing the wrapper for messages

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

A couple small things:

+++ b/core/modules/system/templates/html.html.twigundefined
@@ -36,11 +36,9 @@
-    <div id="skip-link">
       <a href="#main-content" class="visually-hidden focusable">
         {{ 'Skip to main content'|t }}
       </a>
-    </div>

This patch shouldn't be touching html.html.twig.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.phpundefined
@@ -115,7 +115,10 @@ function testThemeSettings() {
-      $elements = $this->xpath('//*[@id=:id]/img', array(':id' => 'logo'));
+      $elements = $this->xpath('//header/a[@rel=:rel]/img', array(
+        ':rel' => 'home',
+        )
+      );
       $this->assertEqual((string) $elements[0]['src'], $expected['src']);
     }
     $unsupported_paths = array(
@@ -163,7 +166,10 @@ function testThemeSettings() {

@@ -163,7 +166,10 @@ function testThemeSettings() {
     $uploaded_filename = 'public://' . $fields[0]['value'];
 
     $this->drupalGet('');
-    $elements = $this->xpath('//*[@id=:id]/img', array(':id' => 'logo'));
+    $elements = $this->xpath('//header/a[@rel=:rel]/img', array(
+        ':rel' => 'home',
+      )
+    );

The first one of these should be indented like the second one (the rel line), and the second last parenthesis on both should have a trailing comma per http://drupal.org/coding-standards#array.

Edit - Like this:

+    $elements = $this->xpath('//header/a[@rel=:rel]/img', array(
+        ':rel' => 'home',
+      ),
+    );
mortendk’s picture

Status: Needs work » Needs review
FileSize
20.06 KB

@cottser check gonna create a seperate issue to remove the #skip-link stuff
update: #2048679: Remove unessasry <div id="skip-link"> from html.html.twig from stark
& fixed the code towards the coding standards thanx :)

The last submitted patch, page-stark-2011578-onlypage.diff, failed testing.

mortendk’s picture

reroll

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed with dreditor and coder. Everything looks golden. Great job @mortendk!

@Cottser's comment in #48 is inaccurate. This would leave a trailing comma right before the signature closure (ie: for another parameter in the $this->xpath() method). Patch in #51 is correct and RTBC :)

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Good point @Mark Carver, my mistake! Follow-up "evil eyes" review below ;)

Also, has this been manually tested? CSS/markup changes need manual testing per https://drupal.org/core-gates#testing.

+++ b/core/includes/theme.incundefined
@@ -2763,10 +2763,6 @@ function template_preprocess_page(&$variables) {
-      '#attributes' => array(
-        'id' => 'main-menu',
-        'class' => array('links', 'inline', 'clearfix'),
-      ),

@@ -2777,10 +2773,6 @@ function template_preprocess_page(&$variables) {
-      '#attributes' => array(
-        'id' => 'secondary-menu',
-        'class' => array('links', 'inline', 'clearfix'),
-      ),

+++ b/core/themes/bartik/bartik.themeundefined
@@ -40,6 +40,7 @@ function bartik_preprocess_page(&$variables) {
   }
   if (!empty($variables['secondary_menu'])) {
     $variables['secondary_menu']['#attributes']['id'] = 'secondary-menu-links';
+    $variables['secondary_menu']['#attributes']['class'] = array('links', 'inline', 'clearfix');
   }
 
   $site_config = config('system.site');

This removes the 'links inline clearfix' classes from both menus but only adds the classes back to Bartik's secondary menu. Can this change be backed up? I thought we didn't want to change Bartik/Seven markup in these issues.

+++ b/core/modules/system/templates/page.html.twigundefined
@@ -64,39 +64,30 @@
-      <div id="name-and-slogan">
+      <div class="name-and-slogan">
...
+      </div>{# #name-and-slogan #}

The closing comment here should be /.name-and-slogan I think to be consistent with the others.

mortendk’s picture

yes the patch remove the classes from both menu's, and only adds in the classes to the secondary menu - primary menu already got those classes add in the bartik before the patch (yes why not add it twice drupal7 ;) )
bartik

 if (!empty($variables['main_menu'])) {
    $variables['main_menu']['#attributes']['id'] = 'main-menu-links';
    $variables['main_menu']['#attributes']['class'] = array('links', 'clearfix');
  }

So its only added when needed ;)
Downloaded & tested as diff the page.twig before & after the patch - the markup is exactly the same in bartik (as espected)

fixed the {# /.name-and-slogan #}

Added screenshots of seven, stark, bartik & seven overlay pre & post patch as required

Note on the stark screenshots: is now not including the inline class or clearfix & is now showing as list items with no styling, as it should be - stark naked ;)

mortendk’s picture

Status: Needs work » Needs review

boot get to work

mortendk’s picture

fixed clearfix issue that was removed from main & sitename follows the stron / h1 issue
screenshots attached for stark.

mortendk’s picture

Issue summary: View changes

General cleanup (mostly spelling) and add dream markup meta to related

Status: Needs review » Needs work

The last submitted patch, page-stark-2011578-sitename-clearfix.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
20.38 KB

shakes fist at UI LANGUAGE NEGOTIATION bot do you magick

mortendk’s picture

Issue summary: View changes

updated the issue

mortendk’s picture

Issue summary: View changes

updated on the possible issue around stark navigation changes visual look

star-szr’s picture

Issue tags: -Stark, -Twig
FileSize
1.26 KB
20.29 KB

Went through everything again, the template files look great.

+++ b/core/modules/system/templates/page.html.twigundefined
@@ -108,41 +105,55 @@
+      {% if page.highlighted %}
+        {{ page.highlighted }}
+      {% endif %}
...
-      {% if page.highlighted %}<div id="highlighted">{{ page.highlighted }}</div>{% endif %}

Since we no longer have a wrapping div around highlighted or tabs I think we can drop a couple if's to clean things up further.

Also fixed a very minor whitespace discrepancy inside a Twig comment.

star-szr’s picture

Issue tags: +Stark, +Twig

Tag monster eats tags.

Status: Needs review » Needs work
Issue tags: -Stark, -Twig, -dreammarkup

The last submitted patch, 2011578-59.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
Issue tags: +Stark, +Twig, +dreammarkup
joelpittet’s picture

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
@@ -423,7 +423,7 @@ function testUrlLanguageFallback() {
-    $fields = $this->xpath('//p[@id="site-name"]/strong/a[@rel="home" and @href=:url]', $args);
+    $fields = $this->xpath('//strong[@class="site-name"]/strong/a[@rel="home" and @href=:url]', $args);

Just remove the second strong/ tag in the testUrlLanguageFallback xpath and I bet you this would work;)

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
@@ -423,7 +423,7 @@ function testUrlLanguageFallback() {
-    $fields = $this->xpath('//p[@id="site-name"]/strong/a[@rel="home" and @href=:url]', $args);
+    $fields = $this->xpath('//strong[@class="site-name"]/a[@rel="home" and @href=:url]', $args);
soulston’s picture

FileSize
20.28 KB

Rerolled with @joelpittet suggestion of removing second /strong tag.

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. All issues have been addressed. Patch is green, good to go for RTBC

xjm’s picture

#64: page-stark-2011578-64.patch queued for re-testing.

alexpott’s picture

Title: Markup for Stark's page.html.twig + maintenance-page.html.twig » [Change notice] Markup for Stark's page.html.twig + maintenance-page.html.twig
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Okay... let's do this. HTML5 here we come.

Committed cc7c70e and pushed to 8.x. Thanks!

There should be one change notice to cover all the issues that are part of #1980004: [meta] Creating Dream Markup

LewisNyman’s picture

+++ b/core/modules/system/templates/maintenance-page.html.twigundefined
@@ -12,80 +12,69 @@
-<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="{{ language.langcode }}" lang="{{ language.langcode }}" dir="{{ language.dir }}">

Sorry, I was away while this patch got RTBC'd. Is this line correct?

soulston’s picture

This patch seems to mirror the current maintenance page, maybe language or more specifically {{ html_attributes }}, is not defined?:

maintenance-page.html.twig

<!DOCTYPE html>
<html>
  {{ head }}
  <title>{{ head_title }}</title>
  {{ styles }}
  {{ scripts }}
</head>

html.html.twig

<!DOCTYPE html>
<html{{ html_attributes }}>
  <head>
    {{ head }}
    <title>{{ head_title }}</title>
    {{ styles }}
    {{ scripts }}
  </head>
soulston’s picture

Actually, is that another error in the maintenance page, no opening <head> tag?

I guess we don't have tests to check the markup then if this got past the test bot?

Yep, Output below using stark:

<!DOCTYPE html>
<html>
  <meta charset="utf-8" />
<link rel="shortcut icon" href="http://d8dev.local/core/misc/favicon.ico" type="image/vnd.microsoft.icon" />
<meta name="Generator" content="Drupal 8 (http://drupal.org)" />

  <title>sSite under maintenance | d8dev.local</title>
  <link rel="stylesheet" href="http://d8dev.local/core/modules/system/css/system.module.css?mrmlc7" media="all" />
<link rel="stylesheet" href="http://d8dev.local/core/modules/system/css/system.theme.css?mrmlc7" media="all" />
<link rel="stylesheet" href="http://d8dev.local/core/modules/system/css/system.admin.css?mrmlc7" media="all" />
<link rel="stylesheet" href="http://d8dev.local/core/modules/system/css/system.maintenance.css?mrmlc7" media="all" />
<link rel="stylesheet" href="http://d8dev.local/core/themes/stark/css/layout.css?mrmlc7" media="all" />

  
</head>
<body class="maintenance-page in-maintenance no-sidebars">

@LewisNyman, let me know if you are happy to go with the <html{{ html_attributes }}>, as per html.html.twig and I'll produce a new patch since the previous patch already got added. I'm not entirely sure if <html{{ html_attributes }}> contains anything at this point, when I tried it didn't output any info but probably good practise if there are attributes in scope.

eatings’s picture

Soulston, you're right. The maintenance page is missing a head tag. Probably oughta fix that...

Yay, six-character patch!

eatings’s picture

Status: Active » Needs review
jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm that the patch in #71 adds back the missing head tag. Good catch guys!

webchick’s picture

Status: Reviewed & tested by the community » Active

D'oh. :P

Committed and pushed that to 8.x. Good catch! Thanks!

Moving back to active for the change notice... I think the html.attributes stuff should probably be discussed in another issue though; it seems like a less straight-forward fix that probably requires some discussion and this sucker's already ~75 comments.

eatings’s picture

Thanks, Angie -- I've opened a new issue at https://drupal.org/node/2011578#comment-7762747 to clean up the last bit of this.

star-szr’s picture

catch’s picture

catch’s picture

Issue summary: View changes

...

rteijeiro’s picture

Assigned: Unassigned » rteijeiro
Issue summary: View changes

Writing change notice...

rteijeiro’s picture

What do you think about this?

Change notice

Markup for Stark's page.html.twig + maintenance-page.html.twig

Summary

Stark is bloated with unnecessary markup & css that makes it hard for new themes to create the markup & css structure thats needed. Cleaning up Stark will not mean that we remove the markup structure & classes for Bartik.

Changes include:

  • Menu attributes removed from Stark and added to Bartik's template.php.
  • #id's removed if they are useless.
  • Layout css follows the css layout rules.
  • maintenance-page.html.twig includes html5 & follows the same structure as page.html.twig.
  • <div role="main"> got the appropriate tag <main role="main">
  • Before
    <div id="main" role="main" class="clearfix">
    After
    <main role="main">

  • Messages wrapper div removed as it's not necessary.
  • Before

{% if messages %}
  <div id="messages">{{ messages }}</div>
{% endif %}

After

{{ messages }}
  • clearfix removed from the markup for main & added in the css main:after to make sure we don't force that solution into the core markup.
  • Removed styling of <li> as it's not necessary.
  • Removed wrapper div & p around site-name link.
  • Before

    <p id="site-name"><strong>
      <a href="{{ front_page }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
    </strong></p>
    

    After

    <strong class="site-name">
      <a href="{{ front_page }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
    </strong>
    
    rteijeiro’s picture

    Status: Active » Needs review

    Change notice ready for review ;)

    mortendk’s picture

    Status: Needs review » Reviewed & tested by the community

    It looks fine to me

    star-szr’s picture

    I discussed briefly with @rteijeiro on IRC that the change notice should be created to encompass all markup changes and we can just keep adding to it/updating it, as mentioned by @alexpott up in #67. For now this might be as simple as giving it a more generic title.

    So we can add this initial change notice, then for example add information about the changes in #1982256: Clean up html.html.twig markup.

    star-szr’s picture

    (double post)

    webchick’s picture

    Status: Reviewed & tested by the community » Needs work

    That sounds like a "needs work." But please just go ahead and create the change notice, rather than leave it in a comment. Even if it's not 100% perfect, it's still better than searching and not finding it. :)

    rteijeiro’s picture

    Ok, just added Change notice here: https://drupal.org/node/2155761

    Sorry if it's wrong but it's my first change notice :)

    star-szr’s picture

    Title: [Change notice] Markup for Stark's page.html.twig + maintenance-page.html.twig » Markup for Stark's page.html.twig + maintenance-page.html.twig
    Assigned: rteijeiro » Unassigned
    Priority: Major » Normal
    Status: Needs work » Fixed
    Issue tags: -Needs change record

    Thank you @rteijeiro! I made a few revisions, mainly to note that this wasn't about converting markup to Twig but about cleaning up markup. Converting markup to Twig already happened in #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch and is ongoing in #1757550: [Meta] Convert core theme functions to Twig templates :)

    star-szr’s picture

    Also I really want to add a link to this, can someone please do so? It feels wrong to say it follows a set of rules and not reference those rules :P

    Layout CSS follows the CSS layout rules. (source?)

    markhalliwell’s picture

    Done

    Status: Fixed » Closed (fixed)

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