Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As per title, probably changes and discussion here is going to pend on usage of the new HTML5 elements such as <article>, <header>, <time>, <menu> and <footer>
.
Comment | File | Size | Author |
---|---|---|---|
#109 | drupal8.node-template.109.patch | 1.81 KB | sun |
#97 | 1077602-97.patch | 2.24 KB | Jacine |
#94 | drupal8.node-template.94.patch | 2.97 KB | sun |
#92 | node-template-1077602-92.patch | 3.35 KB | scor |
#85 | node-template-1077602-85.patch | 4.33 KB | dcmouyard |
Comments
Comment #1
Everett Zufelt CreditAttribution: Everett Zufelt commentedLooking at this issue. Adding article, header and footer elements not really tricky.
As for time, this could be added in the template, we would have to redo how the author / date is printed, or it could be added to template_preprocess_node() by altering $submitted directly.
Although adding it to the template is a bit more work, it would mean that all of the html5 markup is in the template file. Adding it to the preprocess function means that there would be two places where contrib authors would have to look to remove the html5 elements.
Comment #2
Everett Zufelt CreditAttribution: Everett Zufelt commentedAlso, $content['links'] is really the only thing that I think we have in the footer of a node. Since there may not be content in $content['links'] then I think it would be a good idea to do a test before printing the footer, thoughts?
Comment #3
Jeff Burnz CreditAttribution: Jeff Burnz commentedIn node (and comment) it makes more semantic sense for
<footer>
element to contain meta info - such as date and author. There is a use case for links, e.g. when there is a link to comments (comments are related documents). Its probably worth looking at how<footer>
is being used in the wild. I'm not sure we really need to have two footers in node tpl, just get our biggest semantic bang for your bucks which to me is date and author info.I think we should invent a new theme function for dates/times see: #1094324: Add new theme_functions for consistency - Discussion
Comment #4
Everett Zufelt CreditAttribution: Everett Zufelt commented@Jeff
I was thinking that header would be most appropriate for title / author / date. See http://html5doctor.com/the-header-element/
Where I believe on that page title / author / date is in the <header> of the article. And also explained in one of the examples.
Comment #5
cleaver CreditAttribution: cleaver commentedI think that makes sense as a default. Anything else in that space seems a special case.
The spec says: "A footer typically contains information about its section such as who wrote it, links to related documents, copyright data, and the like." I don't think putting author in the footer would be useful to most people.
I originally thought of the footer as being in a page context, rather than node, but this use seems to make sense to me after looking closer at the spec and some examples.
Comment #6
Jeff Burnz CreditAttribution: Jeff Burnz commentedIts still got to make sense as a styling hook - right?
For example we don't currently do this:
But we do have this:
Which would become:
I discussed the "links" problem in #html5 some time ago and outside of Drupal in other places and some feedback I got was that the redefined
<menu>
element could be a good fit for wrapping links.Currently we don't use any additional wrappers for the title or links - which raises the question - is this markup purely for the sake of semantics or are we going to make sure they are useful as well (as styling hooks).
Comment #7
Everett Zufelt CreditAttribution: Everett Zufelt commented@Jeff
If the links aren't footer than we don't have a footer in node.tpl.php, that I can tell. That's alright, if someone wants to add footer for some other reason they can do it in contrib.
IMO header should be used once at the beginning of the node for title / author / date / etc. Not as a wrapper for each. The three together make up the "header" of the node / article.
As to whether these classes are only for semantics or not, I cannot really address this. What I can't say is that Ideally they can be used for additional style hooks, but not at the expense of unnecessary / inaccurate semantics.
Comment #8
casey CreditAttribution: casey commentedhttp://www.w3.org/TR/html5/sections.html#article-example uses <header> for post info in a first example, but <footer> for author/date information for comments in a second example.
The first example also contains a link "Show comments" in <footer>.
Comment #9
Everett Zufelt CreditAttribution: Everett Zufelt commentedIn the spec it states:
I would rather not leave this up to authoring preference, let's pick an implementation and standardize it.
Comment #10
Jeff Burnz CreditAttribution: Jeff Burnz commented@Everett, yes I know what you mean, however that is the problem - wrapping the title, author and date in one header element doesn't make a whole lot of sense from a designers perspective - it's much more flexible if the title and submitted information is separate. In my example I show the current approach and there are good reasons why title and submitted are separate - because its flexible for themers. There are no ideal semantics here, HTML5 gives us leeway, we can use that to our advantage and build templates that are not only semantically rich, but also great for themers. That is the "ideal" we should be striving for.
@casey - yes, great isn't it. I think this is one of the best things about HTML5, its very flexible and its up to us to define the semantics, we're not locked into a single, heavy handed specification.
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedSo, I'm confused. What should the focus of this issue be?
1. Supplying the bare minimum of html formatting so that a node can be rendered?
2. Providing a recommended reference implementation for how a themer should render a node?
3. Providing the best most implementation we can think of for rendering a node?
If 1 and 2) we should simply aspire to create a node.tpl.php that "works" and implements whatever it needs to in order to use the semantic markup so that search engines can gobble up all the stuff nodes spit out.
Comment #12
JacineOk, so we are not just looking to throw together any implementation that just "works" and search engines should have zero to do with this.
The goal here is to make the best, most semantic implementation we can. It needs to be a sensible default and if we do really well, many themes wont bother overriding it. This means it also has to make sense from a design perspective for the majority of use cases.
I think we should do some research into how others are marking posts up. There's a slew of sites that are all nicely categorized here: http://html5gallery.com. We can pick some good examples, post them here as "templates" and discuss what's best for our own implementation.
Does that sound okay?
Comment #13
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, here is one from http://bit.ly/o3ve3p - this is actually a Drupal site.
Thing to note about this example is that the h1 is inside the article - this is the full view, not a teaser view.
Comment #14
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis next one is from http://openconcept.ca also a drupal site and built using one of my theme (full disclosure of course).
The first code block is the teaser, the second is the full node view - on the teaser we have the node title in the article but on the full view its using the standard page title found in most Drupal themes - this is weak IMO because it leaves an untitled section - this is one thing I would really like to see "fixed" in D8.
With additional wrappers and field markup there is a lot more markup in this Drupal 7 example.
The menu element was chosen after consultation with accessibility experts who thought this would provide superior semantics in the long run and not hurt anything now (because it wraps a UL).
Note the use of WAI ARIA roles and microformats. datetime attribute has a proper valid date string also.
Teaser view:
Full node view:
Comment #15
JacineHere's one from Bruce Lawson's blog:
Comment #16
JacineI much prefer the structure of 13 as opposed to 14.
The usage of
<menu>
seems off to me for post links. I don't think post links are like "commands" at all, except for maybe"add comment." I think
<menu>
makes more sense for the Toolbar, Contextual links and Action links than in this case. Also, this hasn't been implemented yet, but according to the spec once it is, this could easily bite us in the ass. See the pic below the code. Here's some more on this from HTML5 Doctor:Another thing I don't like about 14 is the use of
<footer>
for the submitted information at the top of the post. I know that technically this is just as correct as using<header>
, but it's a WTF as far as I'm concerned nonetheless.Some combination of all 3 would probably be best.
Comment #17
Jeff Burnz CreditAttribution: Jeff Burnz commentedDo we even need header and footer elements? I know lots of people are using them, but do we really "need" them?
Also about h2 - in the first example above the heading is in the article, even on full node view, this creates a properly titled section, without this the outline broken because page title is always served from page.tpl.php (untitled section) - we need to rethink this I believe, and have the title always in the article, and shift the logic to page template.
Comment #18
JacineWe probably don't need the header and footer elements, but I think they do make sense semantically. It's a good question. And yes, I agree that we need to move the title into the node template permanently to solve sectioning issues.
Comment #19
skottler CreditAttribution: skottler commentedHere is a first go-round. It requires some tweaking of the attribute, though. Its loosely based on #13.
Comment #20
skottler CreditAttribution: skottler commentedApparently the indentation in Vim is totally broken. I will update with a less-borked patch.
Comment #21
Everett Zufelt CreditAttribution: Everett Zufelt commentedLooks good.
I like the use of header and footer. I think that the links should be in the footer, but the comments likely should not. IMO comments aren't part of the footer of te article, but are a separate sub section of the article.
Comment #22
skottler CreditAttribution: skottler commentedAt second pass, do you think it makes sense to make the primary content area of the node use the
section
tag?Comment #23
skottler CreditAttribution: skottler commentedHere is the corrected patch using the article tag instead of a div for the primary content area.
Comment #24
skottler CreditAttribution: skottler commentedComment #25
sanduhrs#22 I don't think so, per definition everything outside of a sectioning tag is the main content.
So if you have content in an article tag everything in there that is not inside another sectioning tag is the content.
Since article is a sectioning element, we can use h1 for the title here.
See http://dev.w3.org/html5/spec/Overview.html#the-article-element
The link also contains a nice example of how to include the comments.
Also the header element should probably be conditional otherwise it could be empty when all of the contained variables are empty. Same for the footer
Please use spaces instead of tabs to indent.
Comment #26
matglas86 CreditAttribution: matglas86 commentedI reviewed the patch and found 2 minor things. So here is a new patch.
1. Indent on line 97 & 98 was incorrect.
2. The first article tag was not closed.
Besides that I think the structure looks good. One thought I had was that it might be a good idea to implement the summary tag for when a teaser is build. It would need to be researched to see whats the best option to do this.
Comment #27
Everett Zufelt CreditAttribution: Everett Zufelt commentedTook a quick look at the patch.
1. Shouldn't the title be an h1, as this is likely the main title on the page, unless it is a summary, in which case it should be an h2. Note, I don't personally believe that AT / UAs are ready for the headings / sectioning to work by spec in html5
2. Is there a reason that links were taken out of the footer, and that comments are in. Semantically are comments a footer? I thought that links would be good in the footer, and comments should be their own sub-section.
Thanks
Comment #28
JacineRe: #26:
<details>
or<summary>
tags are interactive elements with and open and closed states. They will behave exactly like collapsible fieldsets in Drupal, but natively. This has only been implemented in Chrome thus far and definitely isn't right for this. See this HTML5 Doctor article for more on this.Re: #27:
Yes, I agree with this. The current recommendation is to use standard heading levels because it will pretty much destroy the accessibility of web pages and no browser has actually implemented HTML5 sectioning yet. There was an article about this just 11 days ago on Smashing Magazine. It states the following, emphasis mine:
Bottom line it should be an
<h2>
for teasers.I agree with this too. The links should be in the
<footer>
as I see it, and the comments belong in a<section>
(inside the article tag).Comment #29
Jeff Burnz CreditAttribution: Jeff Burnz commentedComment wrapper uses the section element so no need for any extra markup around comments in node tpl. Agreed that footer should only contain the links.
This is strange, article is a sectioning element that wraps the whole article, this should just be a div - its purely a styling hook in this position.
I don't see a closing article element, and there is no last line.
3 days to next Drupal core point release.
Comment #30
cosmicdreams CreditAttribution: cosmicdreams commentedHere is a patch with the changes requested in #29
Comment #31
cosmicdreams CreditAttribution: cosmicdreams commentedComment #32
JacineComment #33
JacineWhoops. It's not September. LOL.
Comment #34
dcmouyard CreditAttribution: dcmouyard commentedI don't think the <header> element is necessary here, and it definitely shouldn't include $user_picture, $title_prefix, or $title_suffix.
I also don't think the <footer> element should be used for comments. As Jeff mentioned before, the comments already have a sectioning element in comment-wrapper.tpl.php. I think the article metadata should go in <footer>, such as $user_picture and $submitted.
For the node links, I lean towards using <nav> and including a heading for accessibility.
Comment #35
dcmouyard CreditAttribution: dcmouyard commentedHere's a patch that incorporates my ideas from #34.
This is my first Drupal core patch, so I'm crossing my fingers...
Comment #36
dcmouyard CreditAttribution: dcmouyard commentedWhoops! Misplaced a " on line 96.
Comment #37
Jacine@dcmouyard's patch in #36 is growing on me... Seems like a good mix of all the other examples. What do you guys think?
Comment #38
JacineComment #39
jasonsavino CreditAttribution: jasonsavino commentedI believe that the choice between section and article needs to be addressed. IMHO I would use section for full view_mode and article for teaser view_mode. Here is my take on node.tpl.php
Comment #40
Everett Zufelt CreditAttribution: Everett Zufelt commented@jasonrsavino
Can you please explain why you would use section / article as you have stated?
Comment #41
klausiuse spaces instead of tabs, remove trailing white spaces.
Comment #42
jasonsavino CreditAttribution: jasonsavino commented@Everett Zufelt oops! i see the confusion, I inadvertently reversed the section and article tags in my explanation and code. I included a new patch file with the update. Maybe it will help clarify but the main idea of the code, however, was to allow for a developer/themer to choose which option to use rather than restricting to one option.
Comment #43
scor CreditAttribution: scor commentedwhy not indenting here? the snippet above
<div class="submitted">
is indented inside theThere is also whitespaces issue which dreditor will help you find.
Comment #44
JacineI worked on this yesterday after assigning myself earlier this week, so here's my patch.
Comment #46
JacineOops, use this one.
Comment #47
JacineTalked about this with @scor & @Jeff Burnz - Here's a revised and easier to grok version ;)
Comment #49
scor CreditAttribution: scor commented@jacine: the tests are failing because of this. The translation page is expecting a title, and fails otherwise.
-19 days to next Drupal core point release.
Comment #50
scor CreditAttribution: scor commentedLooked at this with @Jacine and others this afternoon. The challenge here is to find a way to get the context of the page: this title removal should only be done on the node view page (node/1) and not on any other tab (node/1/edit). In other words:
!arg(2)
. Of course we want a better way than arg() for doing this, but that's the idea...Comment #51
Jacine@webchick gave us a decent solution here. We'll also be back porting this part to Drupal 7.
Please review ;)
Comment #52
JacineMan, I am completely useless at sprints. New patch uses preprocess (where this should have been in the first place - I just forgot to switch back when testing), simplifies the code and comment in there and fixes the comment at the top of the node template.
Comment #53
skottler CreditAttribution: skottler commentedThis is really great. I love the simplicity of the changes. Going to let the RDF team/others RTBC, but this is RTBC'ed from me.
Comment #54
scor CreditAttribution: scor commentedNot meaning to offend themers, but aren't we trying to keep the PHP level in tpls as low as possible? It seems we might be introducing a new PHP pattern in this tpl. No other tpl from core has this if construct where a variable is set and checked if it's TRUE at the same time. In fact, I could not find any other tpl in core with the regular if '==' pattern:
if ($var == 'value')
. The following is what tpl usually look like:Will test this patch in the meantime and check the markup from an RDFa standpoint.
Comment #55
skottler CreditAttribution: skottler commentedThis line introduces an accidental assignment bug.
Can't we just
print render($content['links']);
-25 days to next Drupal core point release.
Comment #56
webchickWell, no, because you don't want empty
<nav>
tags if there are no links. What does 'accidental assignment' bug mean? It's a deliberate assignment.Comment #57
webchickOh. Although we could move
<nav>
to theme_links() I guess.Comment #58
JacineThis is actually very much needed:
Without it, you'll ALWAYS get the
<nav>
tag whether you've got links or not. This is related to #953034: [meta] Themes improperly check renderable arrays when determining visibility. This issue applies to any render arrays that need a wrapper tag. I had to do the same thing in the comment issue. Basically core is very buggy right now.Can you please clarify what you mean by this? ;) Thanks!
Comment #59
JacineHa! Crosspost ;)
This is a possibility, but I'd rather leave it here in node.tpl.php. It's not a required wrapper, so if people want to change it, it's a lot easier to change it in here than it is to override theme_links(). We'll also probably have a configurable wrapper tag in theme_links() which will make it that much more complicated for people, so I don't know.
Comment #60
webchickOk, fair enough. :)
Comment #61
skottler CreditAttribution: skottler commented@webchick, @jacine: assigning a variable inside of a conditional can lead to issues.
If other people are okay with it, then I'm fine with it.
Comment #62
scor CreditAttribution: scor commentedRDFa markup works great. I've also created #1323830: Place title RDFa metadata inside entity HTML element since this patch will allow us to remove some code from rdf.module :)
We might want to update Bartik at some point up, as a follow up patch or separate issue.
Comment #63
JohnAlbinI'm pretty sure that skottler assumed that we meant to do a
==
equality check, which is why he called our=
an “accidental assignment” which would have been a bug. But, no, we really did mean to do a=
assignment in the if statement as Jacine and webchick pointed out.Comment #64
scor CreditAttribution: scor commentedRight, so @skottler, @Jacine and I discussed this on IRC, and agreed it was better to leave the conditional assignment issue as a follow up for #953034: [meta] Themes improperly check renderable arrays when determining visibility. That solves the concerns I had in #54. and re Bartik, this is handled separately in #1179764: Convert Bartik to HTML5. all set for RTBC.
Comment #65
Everett Zufelt CreditAttribution: Everett Zufelt commentedI'm curious about:
I know this is in the current tpl. DOesn't this allow for duplicate ids if the same node is rendered twice on a page? Does this ever happen? If so, even as an edge case, it is bad.
Comment #66
Jacine@Everett it does allow for duplicate ids in that case, and it's not great. It can be worked around by removing the ID and adding it to the class attribute. This has happened to me in practice only one time, so I'd definitely categorize it as an edge case. It may be possible to use drupal_html_id() in the template, but I'm not positive, and in that case we are better off not providing ID's at all IMO because they'd be unreliable style hooks. So, basically I'd leave it be.
Comment #67
catchThis makes me uncomfortable, since it's leaking assumptions about node rendering into the page as a whole. However while we still have drupal_set_title() and side effects like that I don't see much we can do about it for now.
Committed/pushed to 8.x, looks like this needs porting to 7.x.
Comment #68
catchComment #69
yched CreditAttribution: yched commentedThe node_preprocess_page() code pointed by @catch also makes me tickle a little. That's very node-specific, and any other entity that has a "title" (or a similar "label") will need to implement a similar trick. Not that I have a better suggestion for now, though...
The logic looks odd here :
- if $display_submitted is TRUE and $title is FALSE, an empty
<h2><a></a></h2>
gets printed ?- if $display_submitted is FALSE and $title is not empty, an empty
<p class="submitted"></p>
gets printed ?-27 days to next Drupal core point release.
Comment #70
catchHmm that's a good point.
I've rolled this back for now. We should also open a follow-up issue for the title issue as well.
Comment #71
JacineUgh, sorry about that @catch and @yched. Duh. Hopefully this patch is better.
Also, this patch removes the
<nav>
around $links because people are arguing against it in the comment issue and these should be consistent. And one more thing.... Should we add the ARIA role=article in this patch?Comment #72
JacineHere's one with the ARIA role added.
Comment #73
JacineOy, same patch as #72 without the whitespace. Thanks @aspilicious!
Comment #74
tstoeckler#69 yched:
It's somehow related, so wanted to point people there: #1067126: Support rendering entities in page mode
Comment #75
JacineI've attached a re-roll of this patch due to the core directory commit.
Also, I opened a follow up for the page title issue: #1328048: Page title location needs to be flexible.
Comment #76
rasskull CreditAttribution: rasskull commented#75 patch applied cleanly for me
Comment #77
dcmouyard CreditAttribution: dcmouyard commentedI'll review the patch in #75.
Comment #78
dcmouyard CreditAttribution: dcmouyard commentedI think it’s odd that
$user_picture
gets displayed before the title. I can imagine screen reader users getting confused about hearing “username’s picture” before the title is read.I would prefer to group
$user_picture
and$submitted
together in a<footer>
because it makes more semantic sense to me.I assume we’ll tackle the
<time>
and pubdate stuff later…Everything else looks good. If nobody else agrees with my comments above, then ignore my bikeshedding and mark this RTBC.
Comment #79
rasskull CreditAttribution: rasskull commentedHaving done quite a bit of theme work, this idea scares me a little bit as it sounds like it would complicate things from a presentation standpoint. I imagine countless hours of floating divs, setting left margins along with all sorts of other css hackery to get the image on the left of the post haha.
With that being said, dcmouyard you have a good point about the screen reader, so perhaps someone has another solution that would address both? If this something that was deemed not important, please disregard and move on. :)
Comment #80
JacineYep, this is the precise reason it's at the very top, so that it makes sense from a CSS perspective as well. If we set this up in a way that most people need to override the template just to apply basic styling, IMO we will have failed. There needs to be a happy medium here. I've played with placing the parts of this template in numerous ways, and while none of them are perfect, the proposed patch was the best I could come up with. The image was also previously at the top of the template, likely for the same reason.
If we didn't have to place an image, or the image was always set at a specific small size, this would be a lot easier and more semantic, but we do have to deal with it and need to go with something that makes sense for both styling and semantics.
Comment #81
Everett Zufelt CreditAttribution: Everett Zufelt commentedCan I suggest that we just fix the issue pointed out in #69, and then do follow-ups for the rest?
This would make it a lot easier to deal with small points of contention.
Comment #82
RobLoachSo, the title appears semantically where we want it, but it looks a bit weird underneath the operation tabs on the full page. Not a big problem, just something I'm not quite use to yet. See the attached screenshot.
If we have to do this, we should probably do it where it becomes relevant, down below.
Could probably have a nest of ifs here to just clean it up a bit.
I'm not sure how I feel about segregating out the links and the comments here. I feel like we should just do render($content), and then provide an appropriate wrapper around each theme function within the system itself. The weighting of the links and comments is already done for us.
For example, for the links, stick a #theme_wrapper on it in node_build_content. For the comments, let's try to decouple them from Node and use comment-wrapper.tpl.php.
28 days to next Drupal core point release.
Comment #83
dcmouyard CreditAttribution: dcmouyard commentedThe Bartik theme overrides node.tpl.php and combines the user picture and submitted info below the title in a
<div class="meta submitted">
.Also, the Bartik theme no longer displays the title on individual node pages. So we need to update it’s node template to include the new if statement for displaying the title.
Comment #84
dcmouyard CreditAttribution: dcmouyard commentedFor the patch in #82, shouldn't the title prefix and suffix be rendered even if there’s no title?
Also, I think the comments should be rendered outside the content div. I'm on the fence about whether the node links should be in the content div, but I think we should render it separately to add a heading to the node links.
Comment #85
dcmouyard CreditAttribution: dcmouyard commentedFirst, I agree with Everett in working on the problem pointed out in #69 separately. Perhaps in a separate issue? We should probably fix that issue before continuing with this one
In any case, here’s a summary of changes made in this patch:
Displays title prefix & suffix even if there’s no title.
Removed link around the title when displayed on an individual node page.
Added
rel="bookmark"
to title link.Moved user picture below the title.
Displays user picture and submitted info in a
<footer class="submitted">
.Added a header with
class="element-invisible"
to the node links for accessibility. The heading level changes depending on whether it’s on a page.Added title if/else statement to Bartik’s node template.
Finally, I don’t think these changes should be backported to D7 because existing themes won’t display the title on individual node pages. But this should be hammered out when dealing with the issue in #69.
Comment #86
dcmouyard CreditAttribution: dcmouyard commentedComment #87
rasskull CreditAttribution: rasskull commentedWhat's the procedure on updating the Bartik theme? Either we hit those tpl's at the same time as the core ones, or wait till all the core ones are finalized would be my suggestion. If we start updating those in the same patch as core files, it could become a sticky situation :)
Comment #88
dcmouyard CreditAttribution: dcmouyard commentedI did a quick check to see how the most popular D7 themes handle the user picture.
The following themes output the user picture after the node title:
The following themes output the user picture before the node title:
And Tao doesn't even display the user picture.
Comment #89
rasskull CreditAttribution: rasskull commentedBartik also seems to show title at the top, then groups user photo inside the submitted area. This might be foolish of me to say, but when was the last time anyone saw a user photo next to a main post? I've been looking around and haven't found one instance that wasn't a comment.
Comment #90
sunSee also #1292470: Convert user pictures to Image Field
Comment #91
JacineA few things...
This is a change that would need to be discussed elsewhere, and should come out of this patch. There's already an issue to make all attributes happen in one variable in preprocess.
This doesn't fly with me. First of all, I don't think we need a div or an invisible heading here. This title will not make sense in the majority of cases and the code is a tad too complex for the template. I also don't like naming this $node_links, over links when the variable is called 'links' the $content.
28 days to next Drupal core point release.
Comment #92
scor CreditAttribution: scor commented@rasskull: Bartik will be taken care of in #1179764: Convert Bartik to HTML5. I removed the Bartik hunk from the patch.
That does not solve the issue where the same node teaser is displayed multiple times on the same page. I agree with Jacine it's off topic for this particular issue. #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess() is the issue for the code above, though as I said it doesn't solve all the problems.
I've kept the 'Article links' invisible title for now until we get feedback from more people, and made it a bit less complex.
Comment #93
sunI've tried to implement the suggested "Move page title into node page template" idea back in Drupal 5 already, and all I can say out of that experience is that it blows up and doesn't work out at all. It was horrible for theming.
Especially if it's going to be limited to nodes..... but I guess that's just supposed to be a temporary glitch here, right...?
Anyway, I think we need to rethink the main page content at a larger scope first.
There's much more stuff between the page title and the node content. Say hello to breadcrumb, tabs, help, and whatever other arbitrary stuff in content_top.
Whether there is anything or nothing in between depends on your actual theme, and only on that.
We might be able to solve this challenge by introducing a new
content.tpl.php
-- a "container" that acts as a wrapper for the main page content output on all pages in Drupal.In fugly other words, kinda of an improved system_main block. Treating the actual primary page content similar to other blocks on the page (hence, forming the
<article>
). But without comments. And without a node listing on node listing pages. Just the title, tabs(, help), and the main content output (fields, a form, whatever). [Note that tabs are just a different representation of contextual links -- Contextual module basically had that design idea right from the start.]That's one way to approach this. Another one would be:
Turn the page title, tabs, and main content into blocks, and introduce a main Article region (that kinda lives within the Content region), so modules and site builders can populate that Article region as they wish, but clearly differentiating it from anything else (node listing, comments, etc) in that $content container to make up the primary
<article>
.I guess what all of this boils down to is that we currently have only one notion of $content - but no way at all to allow modules to differ between the main article and "add-on" stuff on and below it. Everything returned from the page callback is stuffed into $content, and the page callback has no chance to say: "Hey, this is right there on the page, but it's NOT the main article piece..."
Now, I totally know that all of this doesn't belong into this issue and discussion. But in order to properly implement this here, we absolutely need to have that discussion and find a proper solution. If anyone can make any sense of what I've written above, I'd appreciate it if you could take that on. :)
However, the approach of the current patch doesn't fly at all for me.
To not block progress here, I'd suggest to simply leave everything alone, but output the node title twice and add an
.element-hidden
to the duplicate title in node.tpl.php.This means there's a well-formed and self-contained
<article>
on the page.(Still leaves us with moving out comments out of that, but as mentioned before, that's a separate issue anyway already.)
Comment #94
sunLike this.
Comment #95
JacineI don't think this discussion should happen here. It's much bigger and far reaching than the node template and at this point, this issue is a complete and utter bike-shedding disaster already. I'd like to see #1328048: Page title location needs to be flexible be the issue where we attempt to address that.
Um, no way. That is no solution, not even a temporary one.
The rest of my feedback in #91 still stands:
This doesn't fly with me. First of all, I don't think we need a div or an invisible heading here. This title will not make sense in the majority of cases and the code is a tad too complex for the template. I also don't like naming this $node_links, over links when the variable is called 'links' the $content.
24 days to next Drupal core point release.
Comment #96
nimbupani CreditAttribution: nimbupani commented+1ing what Jacine said. I dont understand why there needs to be an element-invisible class and a heading for what are essentially node links. This also has nothing to do with HTML5.
Comment #97
JacineHere's a new patch. It does the following:
<h1>
from the template per @sun's comment in #93. We can deal with that in #1328048: Page title location needs to be flexible and it wont be the end of the world.<footer>
element. I'm compromising on this because it actually fixes a bug. The user picture should not print if$display_submitted
is false.$submitted
line in a<p>
.The entire template with the patch applied, looks like this:
It would be great to get this RTBC before 100 comments, because it's getting ridiculous at this point.
Comment #98
Everett Zufelt CreditAttribution: Everett Zufelt commentedLooks great.
1. I would prefer header be used rather than footer, but can form consensus w/ footer.
2. Agreed that there is no need for a heading prior to links in this context.
Comment #99
JacineThanks @Everett! I appreciate the feedback on that.
Regarding
<header>
vs.<footer>
, both are okay. I personally prefer<header>
, as I've said a few times in this issue, but the spec description of<footer>
is arguably more appropriate. Here's the description of both:<footer>
definition from the spec:<header>
definition from the spec:Comment #100
Everett Zufelt CreditAttribution: Everett Zufelt commented+1
Comment #101
dcmouyard CreditAttribution: dcmouyard commentedJacine’s patch in #97 looks good to me.
Comment #102
Jeff Burnz CreditAttribution: Jeff Burnz commentedLooks awesome +1 rtbc.
Comment #103
rasskull CreditAttribution: rasskull commentedLooks great, and yay post 101 rtbc =]
Comment #104
cosmicdreams CreditAttribution: cosmicdreams commentedI'll test this patch this weekend of the act of manually testing it will help it get committed
Comment #105
webchickThis definitely does not need backport to D7. :) Untagging.
Comment #106
catchIf the tag goes, we need a separate issue for backporting the fix from #51 - can someone open that so it doesn't get lost? I'm not sure why that in particular needs to be backported tbh so not sure what to call that issue.
Comment #107
webchickRoger that. Created #1339870: Make the current view mode available on the node object, postponed on this issue.
Also bumping this to "major" instead of "normal". This issue is actually essential to the efforts of the HTML 5 initiative.
Comment #108
catchAlso $node->view_mode is set in the patch, but then never used anywhere. Nor does the comment explain why it's being set.
Comment #109
sunRemoved that hunk.
This is needs to be done properly in #1339870: Make the current view mode available on the node object.
Comment #110
catchMakes sense to do it in that issue, which I've bumped to 8.x, so I've committed/pushed this one to 8.x. Thanks!
Comment #115
xjm