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
Comment | File | Size | Author |
---|---|---|---|
#71 | twig-maint-page-head-tag-restored.patch | 405 bytes | eatings |
#64 | page-stark-2011578-64.patch | 20.28 KB | soulston |
#59 | 2011578-59.patch | 20.29 KB | star-szr |
#59 | interdiff.txt | 1.26 KB | star-szr |
#58 | page-stark-2011578-donedonedone.diff | 20.38 KB | mortendk |
Comments
Comment #1
LewisNymanThis 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
Comment #2
mortendk CreditAttribution: mortendk commentedyup 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.
Comment #3
LewisNymanYes 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.
Comment #4
mortendk CreditAttribution: mortendk commentednopes 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
Comment #5
mortendk CreditAttribution: mortendk commentedupdated 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 nowso that would kill the l-sidebar-first & l-sidebar-first div wrappers
Comment #6
star-szrTagging.
Comment #7
star-szrComment #8
tlattimore CreditAttribution: tlattimore commentedThis 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.
Comment #10
mortendk CreditAttribution: mortendk commented@tlattimore rerolled it removed the region changes - so we can move forward the regions is gonna be its own issue
Comment #11
mortendk CreditAttribution: mortendk commentedComment #12
mortendk CreditAttribution: mortendk commentedadded tag for stark + twig
Comment #12.0
mortendk CreditAttribution: mortendk commentedbetter description
Comment #14
tlattimore CreditAttribution: tlattimore commentedI will look into what is causing this to fail so many tests.
Comment #15
mortendk CreditAttribution: mortendk commentedyup looks like my ignorence towards test is now my nemesis
Comment #16
tlattimore CreditAttribution: tlattimore commentedHere 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.
Comment #17
tlattimore CreditAttribution: tlattimore commentedTagging 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.
Comment #19
tlattimore CreditAttribution: tlattimore commentedI'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.
Comment #20
tlattimore CreditAttribution: tlattimore commentedHere's is an updated patch that should resolve the remaining test failures.
Comment #21
tlattimore CreditAttribution: tlattimore commentedgo ahead test bot.
Comment #22
mgiffordWhy are you replacing
id
s withclass
es? I'm not sure what additional flexibility it gives you, but feels like a bit of a loss as far as structure goes.Comment #23
LewisNymanIt's part of our new CSS standards:
Comment #24
mgiffordThe 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?
Comment #25
mortendk CreditAttribution: mortendk commentedfirst 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 ;) )
Comment #26
mgiffordI'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:
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/
Comment #27
mortendk CreditAttribution: mortendk commentedGetting 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:
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
Comment #28
mgiffordRegarding "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?
Comment #29
mortendk CreditAttribution: mortendk commentedThis code is for stark - so its clean mean markup + make sure that we dont break bartik.
Comment #30
mortendk CreditAttribution: mortendk commentedwonder 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
Comment #32
mortendk CreditAttribution: mortendk commentedid="messages" remove from the test
Comment #33
mortendk CreditAttribution: mortendk commentedComment #33.0
mortendk CreditAttribution: mortendk commentedreworded
Comment #33.1
mortendk CreditAttribution: mortendk commentedcleaned up description
Comment #33.2
mortendk CreditAttribution: mortendk commentedclearified the text
Comment #34
drupalninja99 CreditAttribution: drupalninja99 commented@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 =).
Comment #35
Brandonian CreditAttribution: Brandonian commentedMarkup looks good, meshes with the new CSS/markup standards. Patch needs to be redone to remove various @todo's within the patch.
Comment #36
mortendk CreditAttribution: mortendk commentedremoved todo's reroll
Comment #37
Brandonian CreditAttribution: Brandonian commentedNew patch looks good. Assuming testbot likes it as well, RTBC.
Comment #38
sjbassett CreditAttribution: sjbassett commentedAdding NYCcamp2013 tag
Comment #39
eatingsA 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?
Comment #40
mortendk CreditAttribution: mortendk commentedThe 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
Comment #41
eatingsApart from the new issue in #40, this looks good.
edit: just waiting for testbot to clear. Otherwise, looks RTBC to me.
Comment #42
LewisNymanI'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.
Do we need the body declaration in the selector?
We could use a common class here like l-sidebar to cut down on repetitive CSS. But also see:
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?
Comment #43
mortendk CreditAttribution: mortendk commentedl-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
Comment #44
LewisNymanThat's fine with me, we can have class naming discussions in a follow up
Comment #45
mortendk CreditAttribution: mortendk commented- 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 livechanges the
<div role="main">
to the appropiate<main role="main>
tag insteadplease review (& testbot show me your magic)
Comment #45.0
mortendk CreditAttribution: mortendk commentedcleaned up a bit more
Comment #46
Brandonian CreditAttribution: Brandonian commentedLooks good to me, and bot likes it. Marking as RTBC.
Comment #47
Brandonian CreditAttribution: Brandonian commentedActually marking RTBC.
Comment #47.0
star-szradded info about main tag + removing the wrapper for messages
Comment #48
star-szrA couple small things:
This patch shouldn't be touching html.html.twig.
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:
Comment #49
mortendk CreditAttribution: mortendk commented@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 :)
Comment #51
mortendk CreditAttribution: mortendk commentedreroll
Comment #52
markhalliwellReviewed 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 :)
Comment #53
star-szrGood 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.
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.
The closing comment here should be /.name-and-slogan I think to be consistent with the others.
Comment #54
mortendk CreditAttribution: mortendk commentedyes 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
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 ;)
Comment #55
mortendk CreditAttribution: mortendk commentedboot get to work
Comment #56
mortendk CreditAttribution: mortendk commentedfixed clearfix issue that was removed from main & sitename follows the stron / h1 issue
screenshots attached for stark.
Comment #56.0
mortendk CreditAttribution: mortendk commentedGeneral cleanup (mostly spelling) and add dream markup meta to related
Comment #58
mortendk CreditAttribution: mortendk commentedshakes fist at UI LANGUAGE NEGOTIATION bot do you magick
Comment #58.0
mortendk CreditAttribution: mortendk commentedupdated the issue
Comment #58.1
mortendk CreditAttribution: mortendk commentedupdated on the possible issue around stark navigation changes visual look
Comment #59
star-szrWent through everything again, the template files look great.
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.
Comment #60
star-szrTag monster eats tags.
Comment #62
markhalliwell#58: page-stark-2011578-donedonedone.diff queued for re-testing.
Comment #63
joelpittetJust remove the second
strong/
tag in the testUrlLanguageFallback xpath and I bet you this would work;)Comment #64
soulston CreditAttribution: soulston commentedRerolled with @joelpittet suggestion of removing second /strong tag.
Comment #65
markhalliwellLooks good to me. All issues have been addressed. Patch is green, good to go for RTBC
Comment #66
xjm#64: page-stark-2011578-64.patch queued for re-testing.
Comment #67
alexpottOkay... 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
Comment #68
LewisNymanSorry, I was away while this patch got RTBC'd. Is this line correct?
Comment #69
soulston CreditAttribution: soulston commentedThis patch seems to mirror the current maintenance page, maybe language or more specifically {{ html_attributes }}, is not defined?:
maintenance-page.html.twig
html.html.twig
Comment #70
soulston CreditAttribution: soulston commentedActually, 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:
@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.
Comment #71
eatingsSoulston, you're right. The maintenance page is missing a head tag. Probably oughta fix that...
Yay, six-character patch!
Comment #72
eatingsComment #73
jenlamptonI can confirm that the patch in #71 adds back the missing head tag. Good catch guys!
Comment #74
webchickD'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.
Comment #75
eatingsThanks, Angie -- I've opened a new issue at https://drupal.org/node/2011578#comment-7762747 to clean up the last bit of this.
Comment #76
star-szrThe issue opened by @eatings is #2067915: Restore html attributes to maintenance-page.html.twig :)
Comment #77
catch#2083415: [META] Write up all outstanding change notices before release
Comment #77.0
catch...
Comment #78
rteijeiro CreditAttribution: rteijeiro commentedWriting change notice...
Comment #79
rteijeiro CreditAttribution: rteijeiro commentedWhat 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:
<div role="main"> got the appropriate tag <main role="main">
Before
<div id="main" role="main" class="clearfix">
After
<main role="main">
Before
After
<li>
as it's not necessary.Before
After
Comment #80
rteijeiro CreditAttribution: rteijeiro commentedChange notice ready for review ;)
Comment #81
mortendk CreditAttribution: mortendk commentedIt looks fine to me
Comment #82
star-szrI 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.
Comment #83
star-szr(double post)
Comment #84
webchickThat 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. :)
Comment #85
rteijeiro CreditAttribution: rteijeiro commentedOk, just added Change notice here: https://drupal.org/node/2155761
Sorry if it's wrong but it's my first change notice :)
Comment #86
star-szrThank 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 :)
Comment #87
star-szrAlso 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
Comment #88
markhalliwellDone