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

Files: 
CommentFileSizeAuthor
#71 twig-maint-page-head-tag-restored.patch405 byteseatings
PASSED: [[SimpleTest]]: [MySQL] 58,043 pass(es).
[ View ]
#64 page-stark-2011578-64.patch20.28 KBsoulston
PASSED: [[SimpleTest]]: [MySQL] 57,906 pass(es).
[ View ]
#59 2011578-59.patch20.29 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 57,486 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#59 interdiff.txt1.26 KBCottser
#58 page-stark-2011578-donedonedone.diff20.38 KBmortendk
FAILED: [[SimpleTest]]: [MySQL] 57,136 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] 57,549 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#51 page-stark-2011578-onlypage2.diff20.06 KBmortendk
PASSED: [[SimpleTest]]: [MySQL] 57,537 pass(es).
[ View ]
#49 page-stark-2011578-onlypage.diff20.06 KBmortendk
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php.
[ View ]
#45 page-stark-2011578-nobody.diff20.57 KBmortendk
PASSED: [[SimpleTest]]: [MySQL] 57,622 pass(es).
[ View ]
#36 page-stark-2011578-notodo.diff20.81 KBmortendk
PASSED: [[SimpleTest]]: [MySQL] 57,046 pass(es).
[ View ]
#32 page-stark-2011578-3.diff21.14 KBmortendk
PASSED: [[SimpleTest]]: [MySQL] 57,105 pass(es).
[ View ]
#30 page-stark-2011578.diff18.48 KBmortendk
FAILED: [[SimpleTest]]: [MySQL] 57,025 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#20 2011578-20.patch20.78 KBtlattimore
PASSED: [[SimpleTest]]: [MySQL] 57,171 pass(es).
[ View ]
#20 interdiff.txt5.01 KBtlattimore
#16 interdiff.txt2.93 KBtlattimore
#16 2011578-16.patch15.97 KBtlattimore
FAILED: [[SimpleTest]]: [MySQL] 55,558 pass(es), 16 fail(s), and 9 exception(s).
[ View ]
#10 2011578-page_and_maintainence3.diff13.86 KBmortendk
FAILED: [[SimpleTest]]: [MySQL] 57,189 pass(es), 32 fail(s), and 2,354 exception(s).
[ View ]
#5 2011578-page_and_maintainence.diff16.96 KBmortendk
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
page+maintainence-page.diff14.06 KBmortendk
FAILED: [[SimpleTest]]: [MySQL] 56,951 pass(es), 22 fail(s), and 10 exception(s).
[ View ]

Comments

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

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.

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.

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

Status:Active» Needs review
StatusFileSize
new16.96 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

Issue tags:+dreammarkup

Tagging.

Title:markup for page.html.twig + maintanence-page.html.twigMarkup for page.html.twig + maintenance-page.html.twig

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.

Status:Needs work» Needs review
StatusFileSize
new13.86 KB
FAILED: [[SimpleTest]]: [MySQL] 57,189 pass(es), 32 fail(s), and 2,354 exception(s).
[ View ]

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

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

Issue tags:+Stark, +Twig

added tag for stark + twig

Issue summary:View changes

better description

Status:Needs review» Needs work

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

Assigned:Unassigned» tlattimore

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

yup looks like my ignorence towards test is now my nemesis

StatusFileSize
new15.97 KB
FAILED: [[SimpleTest]]: [MySQL] 55,558 pass(es), 16 fail(s), and 9 exception(s).
[ View ]
new2.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.

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.

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.

StatusFileSize
new5.01 KB
new20.78 KB
PASSED: [[SimpleTest]]: [MySQL] 57,171 pass(es).
[ View ]

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

Status:Needs work» Needs review

go ahead test bot.

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.

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.

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?

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 ;) )

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/

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

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?

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

StatusFileSize
new18.48 KB
FAILED: [[SimpleTest]]: [MySQL] 57,025 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new21.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,105 pass(es).
[ View ]

id="messages" remove from the test

Status:Needs work» Needs review

Issue summary:View changes

reworded

Issue summary:View changes

cleaned up description

Issue summary:View changes

clearified the text

@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 =).

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.

Status:Needs work» Needs review
StatusFileSize
new20.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,046 pass(es).
[ View ]

removed todo's reroll

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

Adding NYCcamp2013 tag

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?

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

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.

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?

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

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

Status:Needs work» Needs review
StatusFileSize
new20.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,622 pass(es).
[ View ]

- 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)

Issue summary:View changes

cleaned up a bit more

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

Status:Needs review» Reviewed & tested by the community

Actually marking RTBC.

Issue summary:View changes

added info about main tag + removing the wrapper for messages

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',
+      ),
+    );

Status:Needs work» Needs review
StatusFileSize
new20.06 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php.
[ View ]

@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.

StatusFileSize
new20.06 KB
PASSED: [[SimpleTest]]: [MySQL] 57,537 pass(es).
[ View ]

reroll

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 :)

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.

StatusFileSize
new20.06 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new508.74 KB
new177.71 KB
new533.59 KB
new205.78 KB
new204.52 KB
new203.56 KB
new130.6 KB
new164.17 KB

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 ;)

Status:Needs work» Needs review

boot get to work

StatusFileSize
new20.38 KB
FAILED: [[SimpleTest]]: [MySQL] 57,549 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new179.02 KB
new200.31 KB
new177.54 KB
new199.42 KB

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

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.

Status:Needs work» Needs review
StatusFileSize
new20.38 KB
FAILED: [[SimpleTest]]: [MySQL] 57,136 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

shakes fist at UI LANGUAGE NEGOTIATION bot do you magick

Issue summary:View changes

updated the issue

Issue summary:View changes

updated on the possible issue around stark navigation changes visual look

Issue tags:-Stark, -Twig
StatusFileSize
new1.26 KB
new20.29 KB
FAILED: [[SimpleTest]]: [MySQL] 57,486 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

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.

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

#58: page-stark-2011578-donedonedone.diff queued for re-testing.

+++ 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);

StatusFileSize
new20.28 KB
PASSED: [[SimpleTest]]: [MySQL] 57,906 pass(es).
[ View ]

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

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

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

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

+++ 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?

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>

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.

StatusFileSize
new405 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,043 pass(es).
[ View ]

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

Yay, six-character patch!

Status:Active» Needs review

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!

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.

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

Issue summary:View changes

...

Assigned:Unassigned» rteijeiro
Issue summary:View changes

Writing change notice...

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>

Status:Active» Needs review

Change notice ready for review ;)

Status:Needs review» Reviewed & tested by the community

It looks fine to me

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.

(double post)

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. :)

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

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

Title:[Change notice] Markup for Stark's page.html.twig + maintenance-page.html.twigMarkup 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-63] Convert core theme functions to Twig templates :)

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?)

Done

Status:Fixed» Closed (fixed)

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