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.
With the committing of the new page tpl code:
We should now move on to node templates and getting their output to be more useful, and more easily managed.
Let the Bikeshed begin!
Comment | File | Size | Author |
---|---|---|---|
#109 | node-tpl-php-382870-109.patch | 12.84 KB | JohnAlbin |
#108 | node-tpl-php-11.patch | 7.2 KB | geerlingguy |
#106 | node-tpl-php-10.patch | 6.95 KB | geerlingguy |
#103 | node-tpl-php-10.patch | 6.95 KB | geerlingguy |
#100 | node-tpl-php-9.patch | 7.97 KB | geerlingguy |
Comments
Comment #1
geerlingguy CreditAttribution: geerlingguy commentednode.tpl.php, in case anyone is searching for this thread (i.e. me ;-).
Will look into it this evening.
Comment #2
wretched sinner - saved by grace CreditAttribution: wretched sinner - saved by grace commentedComment #3
sunAfter quickly scanning the other issue:
I'm +1000 for unique CSS classes, i.e. .node-title, .node-content, .block-title, .block-content, etc. That's the only way to properly style Drupal's potentially recursive output in a cross-browser-compatible way (since Drupal can produce .node > .block > .node). Generic stuff like .title or .content or .links should die.
Comment #4
webchicksubscribe.
Comment #5
geerlingguy CreditAttribution: geerlingguy commentedRE #3/sun: I'm 100% with you there. One area that could use about 2000x more improvement in this area is the links section.
Comment #6
JohnAlbin/me wonders why he hasn't already subscribed to this. :-p
Comment #7
yoroy CreditAttribution: yoroy commentedLinks section needs some attention indeed. I'd love to see 'read more' and 'add comment' not being lumped together for example.
Maybe a little tag like this for these specific issues?
Comment #8
geerlingguy CreditAttribution: geerlingguy commented@ yoroy: Indeed, the Read more link issues are already being discussed in #16161: Move Read More link to end of node content
Comment #9
geerlingguy CreditAttribution: geerlingguy commentedHere's attempt number 1. Criticism shall commence (constructive, of course)!
Changes (I think these are all...):
Edit: Gaaah! All that typing for naught... I had to retype this post.
Comment #10
webchickOn picture/title, we need classes rather than IDs I think. For example, on the default front page listing you would have several nodes, each with user pictures and titles. XHTML cries in pain when there is more than one ID with the same name on a page. :)
Comment #11
webchickAlso, if you have .node-terms, what's the point of .terms-inline? (I realize that was there before; just curious)
Comment #12
geerlingguy CreditAttribution: geerlingguy commented@ webchick: D'oh! Wasn't even thinking of that! And I just left terms-inline in there because I didn't know exactly why it was there, and didn't want to upset the apple cart...
New patch attached - also added a comment for clarity, and added the .section .clearfix class selectors to the inner part of the node.
Comment #13
JohnAlbinWhile reviewing this from the airport terminal...
The .section class is only to be used in the page.tpl for page sections.
Also, there's already a comments-wrapper.tpl, so there's no need to wrap $comments again.
Comment #14
geerlingguy CreditAttribution: geerlingguy commented@JohnAlbin - thanks for the notes. Here's an updated patch:
Comment #15
kika CreditAttribution: kika commented+ <div id="node-user-picture">
Again, it's not good to use id attribute on likely repeating element.
Comment #16
geerlingguy CreditAttribution: geerlingguy commented@kika - D'oh! Thanks for catching that... I was thinking of the node in terms of one page only. Corrected:
Comment #17
Frando CreditAttribution: Frando commentedI would add a class node-{$node->type} to the main node div.
Comment #18
geerlingguy CreditAttribution: geerlingguy commented@Frando - what would the PHP syntax be for that? Seems to me like a very good idea! More specificity in selectors = more bliss from a designer's perspective.
This is one of the reasons why we're hoping to get the .tpl.php files nailed down—this way designers like me, who are not clueless, but don't know PHP like the back of their hand, don't have to get their fingers sticky with it ;-)
Comment #19
Frando CreditAttribution: Frando commentedprint "node-{$node->type}";
. Or, the better approach would likely be to assemble all classes for the node div in template_preprocess_node and then just pass on a variable $node_classes that will be printed in the template.Comment #20
sunNo, please don't. Themers should not have to lookup other functions to customize the output to their likings or requirements. Instead, we should simplify the usage of variables by always defining them and thereby reduce the PHP logic in templates, i.e.:
becomes
Yes, this will output extra spaces if one or both of the status classes is empty, but that is only a cosmetical issue and not against any standard. (And the cosmetical issue can be fixed by those who care.)
Comment #21
geerlingguy CreditAttribution: geerlingguy commented@sun - I like your suggestion; that's what seems to be the standard across CSS classes, correct? I notice on the front page of one of my sites, there's a nice variety of CSS variables to choose from (and I like it that way):
<body class="front logged-in one-sidebar sidebar-left">
To a non-PHP guy, this kind of system makes a whole lot more sense. I'll re-roll the patch later today when I'm home from work
Comment #22
geerlingguy CreditAttribution: geerlingguy commentedHere's the updated patch - no PHP logic included. Should we have the unpublished class be
node-unpublished
or justunpublished
?Comment #23
geerlingguy CreditAttribution: geerlingguy commentedWhoops! *Here's* the updated patch...
Comment #24
geerlingguy CreditAttribution: geerlingguy commentedGaa! Just noticed a lack of a space in one of the php snippets. Fixed, and republished. (I wish there were a way to retract posts within 5 minutes or something along those lines).
Comment #25
yoroy CreditAttribution: yoroy commentedsetting status
Comment #27
geerlingguy CreditAttribution: geerlingguy commentedHmm... t.d.o said there were two exceptions in the 'Theme interface functionality' - I am scratching my head on this one... did I mess up one of the PHP variables?
Comment #28
geerlingguy CreditAttribution: geerlingguy commentedAttached is a new patch with a few fixes that EclipseGc, webchick, merlinofchaos and I chatted about in IRC. This patch has a little ways to go, but here it is for your pleasure and discussion!
Notes:
Also: After rolling this patch, it was determined we should take out the CSS class
.user-picture
around$picture
innode.tpl.php
, and (with webchick's approval) replace the.picture
class inuser-picture.tpl.php
with.user-picture
. These changes will be rolled into the next version of this patch.Comment #29
sunAs long as we (themers) have to support browsers that do not properly support current CSS standards, we need specific CSS classes. That is, because Drupal can output nodes inside blocks, and inside of the block, a comment. So your ".node .links" selector will match
1) Links of the node
2) Links of the block (there just has to be _something_ that uses the CSS class "links")
3) Links of the comment
But .node-links will only match what you intended, the node's links.
From all improvements we can do to our template markup, I would consider this the most important one.
Comment #30
geerlingguy CreditAttribution: geerlingguy commentedI'm inclined to drop support of IE 6 in Drupal 7's core .tpl.php files... I have stopped supporting it in most of my new site developments, only fixing fundamental issues with layouts, and treating it as a second-class-citizen. A lot of buzz has been going on about this lately, including this nicely-written article on SitePoint, and I think that since Drupal 7 is going to be integrating some forward-looking HTML5 and web accessibility standards, we might as well cut off IE 6 in core.
If you want to support IE 6 or older browsers specifically in your theme, then you can copy out the node.tpl.php and put in as many unique selector names as you want :-)
(I'm not trying to be rude here, I'm just trying to make my point).
Comment #31
EclipseGc CreditAttribution: EclipseGc commented@geerlingguy
yeah nice thought but:
The official line here is that we support what jquery supports, and frankly, IE6 is still over 20% of the market... that's akin to saying "We're not going to support Firefox." which is of course not going to happen.
@sun
the selector div.node .links will match only items with the link class if they appear within div.node. This is as opposed to div.node>links which would match only direct children divs of the node classed div with a class of links on them (and this is unsupported by IE6, the original selector however works just fine). If comments are currently being put inside that div.node, then that's something we should fix (though I can see the argument that they're this node's comments... but we can hash that out later if this is the case). Point being div.node .links should match nothing in a block unless there's a div.node inside the block (which is possible with views) but that would be a teaser or full node view, and is going to match any html we drop on it here.
Point being we can reuse .links to our hearts content and have a single style for ALL instances or more specific styles for nodes, comments, blocks etc. This is the core intent of css, and I don't think we're using it incorrectly here (or I've completely misunderstood your point, if so please clarify for me)
Eclipse
Comment #32
geerlingguy CreditAttribution: geerlingguy commented@ EclipseGc - I agree the words "cut off" (as I stated in my previous comment) might be a little much... but would you agree we could treat IE6 as a 'second class citizen'? A lot of little javascript functions degrade in IE6, and as long as the fundamental layout selectors are accessible by IE6 (which they are, right now), we can move forward on smaller things...
Comment #33
sunWell. I can certainly continue to use these classes in my custom themes (and I will). I just wanted to point out that it took me a fair amount of time to figure out how to make it work. As long as IE6 is in the game, and you do not want to (be forced to) use a JavaScript helper (like Dean Edward's IE7 JavaScript, which I used to work around this issue before figuring it out), using specific classes for rendered entities is the only way to account for a proper styling.
The cause is that Drupal and especially contrib modules can output data belonging to different entities _recursively_. So your innocent
.node .title
CSS selector (also .content and .links) will match each and everything that uses this class and is rendered below a .node entity. Whether that is a block with a title (or .content or .links), a view with arbitrary entities (and sub-entities), a comment, a trackback, or whatever does not matter. Of course, CSS styles of each .<entity> .title are merged, so you end up with nasty CSS to revert the styles that were intended for the parent entity.Example:
Much cleaner:
I can certainly understand the idea of using common class names for things that ought to be the same. However, in 99% of all cases (I experienced with designs and themes so far), each entity has its own styling, including the output that belongs to the entity - but not recursively.
Comment #34
EclipseGc CreditAttribution: EclipseGc commentedok, well
.node .title {
style:here;
}
will not have any bleed over into
.block .title {
style:here
}
unless there is a classed structure of:
.block .node .title or .node .block .title
what .node .title says is any element with the class title that appears ANYWHERE inside of an element with a class of node. So that's infinite levels deep. so if that node were to be printed in a block, it would retain the same styling as it does when it appears in $content.
What this allows us to do is obviously we can simply set:
.title {
sane:defaults;
}
and then get more specific for nodes or blocks or comments or whatever. However if we start off the with more specific and don't make use of the less specific, there should be no styles inherited as you have outlined in your post.
Comment #35
Jeff Burnz CreditAttribution: Jeff Burnz commentedWhy is the h2 wrapped in a div? To me this is redundant and something perhaps a designer *may* need but certainly not a default.
Re: high specificity selectors—I have used high specificity selectors for node, block & comment titles in all my recent theming (over 40 Genesis sub themes) and can't recall doing:
But I can if I need to, however unlikely.
Any "sane defaults" cascade from resetting the type selector, e.g. h2 margins, padding, line-height & perhaps font; generally I only use h2 for node and block titles.
I am a huge +1 for the more specific selectors, it just makes life a whole lot easier in the day to day grind of building Drupal themes. I just don't see the need to force the use of deep descendant selectors just to style a block title because it happens to be in a node or vice versa.
Comment #36
geerlingguy CreditAttribution: geerlingguy commentedOn the tail end of #35/jmburnz' comment, I would argue that having nodes in blocks and vice versa is not something that we need to be supporting in core, simply because that seems to me to not be a typical use scenario (so far, I haven't done that on more than one site I've Drupalized). In addition, you can't even do this without using contrib/custom modules.
Therefore if someone wants to do something like that, they can customize their theme a little more heavily. I think by the time someone's getting into putting nodes into blocks and blocks into nodes, they will probably need to do some other customizations that will be harder/require more skill than copying out/modifying page.tpl.php.
Comment #37
ejort CreditAttribution: ejort commentedI also agree that I've never had the need to set common styles for block and node titles etc. However, a happy medium could be to specify multiple classes so that you can style the generic class or the more specific one.
ie/
<div class="title node-title"><h2>...</h2></div>
Comment #38
Jeff Burnz CreditAttribution: Jeff Burnz commented@#36, Views is the most popular module which makes nodes in blocks trivial. Its not about what we should or should not support in core, its about making theming easier & our CSS cleaner, see example in #33.
You can always do both, however I would argue that .title is redundant in this scenario given there exists several other selectors to cascade styles to node and block h2.
Not so my friend, placing blocks in nodes is a trivial as one line in phptemplate_preprocess_node, a node in a block ain't a whole lot more.
Comment #39
EclipseGc CreditAttribution: EclipseGc commentedok, maybe the intent of this work was lost, so I'm going to give us a quick "re-alignment" towards our original goals.
The original goal of this project (i.e. rebuilding all the tpl files in core) is to provide better, cleaner css only theme integration on top of cores existing tpl files. This means cleaner class structure (i.e. as little as is feasible) cleaner html output (again as little as reasonable) and sensible defaults.
Why I'm against .node-title
.node-title is actually less specific than .node .title being 1 class vs 2 classes (0,1,0 vs 0,2,0). For those of you unfamiliar with the number I just put up you should visit http://www.stuffandnonsense.co.uk/archives/images/specificitywars-05v2.jpg .
We could potentially select on .node h2 but that is a (0,1,1) again less specific and this doesn't allow us to place a sensible css default on ALL titles (something .node-title also prevents). I'm against class="title node-title" as well because this is extra markup for no specific purpose. Again .node-title is actually less specific, and with the prevalence of firebug and products similar to it, the argument against class in class selectors seems lame to me. On the page.tpl.php we worked very hard to get only the class structures we felt we needed. I will do the same here, so let me state this for the record.
We have no need of "node-" classes as all these subclasses will already be contained within a node class, and .node .subclass is more specific, common practice, and a better solution.
So unless someone has a really good rebuttal for something I've said here, let's keep moving foward and bikeshed on this topic no more.
Eclipse
Comment #40
sunSure, we can simply ignore the reality. And keep cross-browser theming hard for themers.
I just wanted to point out what could be done to make life of themers actually easier.
Where can I unsubscribe from this issue?
Comment #41
akahn CreditAttribution: akahn commentedSun, can you go into more detail about the cross-browser support issues here? I understand the
.node .links { font-family: Foo, Bar; }
affecting comment links concern but not how this poses problems for cross-browser styling.Overall, I think what EclipseGc is going for makes a lot of sense and follows the spirit of CSS and of separating style from content. Style .title in the general sense, and then style the more specific cases of .title. This makes it possible to craft clean, well-organized stylesheets.
Comment #42
sun.node .links
certainly is the "proper" way. If you refer to standards. If you want to support modern browsers. If you do not have to support older browsers that are still being used. If you think that nasty style overrides and resets are acceptable. Or if you are ok with loading a bloat of JS to make older browsers behave like modern browsers.Comment #43
EclipseGc CreditAttribution: EclipseGc commented@sun
just for clarity sake when you say "older" you mean IE6? I'm just trying to make sure we're all on the same page.
Eclipse
Comment #44
sunYes, primarily targeting IE6. Any browser that does not support
.node > .title
, because that's the actual issue for why.node-title
makes a themer's life a lot easier.Comment #45
EclipseGc CreditAttribution: EclipseGc commentedok, so the more we talk the more I feel like we're having a huge misunderstanding somewhere.
1.) yes you're absolutely correct ie6 doesn't support direct child selectors.
2.) I'm not sure why this is relevant with the one possible exception of comments (which needs to be fixed and I see as kind of separate from this issue)
3.) What am I missing here?
Thanks,
Eclipse
Comment #46
Jeff Burnz CreditAttribution: Jeff Burnz commentedI have put together a graphic illustration of why the use of .node-title and .block-title makes sense.
#col-1 is very simple and shows how .title inherits attributes.
#col-2 shows how we are forced to use deep descendant selectors for correct display across browser.
#col-3 uses .node-title and .block-title and clearly is the least amount of CSS AND displays correctly across browser.
#col-4 you will see that IE6 chokes on the child selector and is thus not a viable option.
EDIT: ops, left some crap in the file, does not affect the display though, just some notes I was keeping in the paragraphs...
Comment #47
webchickDisclaimer: I am not a themer.
I just want to point out that Drupal 7 will in all likelihood not be out before Q1 2010. Contrib modules not up to speed not before Q2 2010, which means people aren't going to be actually putting Drupal 7 sites out into the wild until Q2/Q3 2010. Drupal 7 will be EOLed when Drupal 9 comes out, which will not be before 2012, possibly much later, at which point IE6 will be 11 years old. And IE 8 came out the other day.
Bearing all of this in mind, does it really make sense to hold back Drupal's *default* styling (bearing in mind this is all overridable on a per-theme and per-client basis) because of IE 6, given that the worst that is going to happen it seems to be that they'll see goofy colours they shouldn't?
I'm perfectly fine with the answer to this question being "yes" btw; as I said, I'm ignorant.
Comment #48
geerlingguy CreditAttribution: geerlingguy commentedRE #37-47: I think we can go one of two ways:
1. (This will not please sun or jmburnz ;-) include only .title, .whatever, etc. in node.tpl.php.
2. (This = code bloat, imo, but would please everyone—sort of) include .title and .node-title, .whatever and .node-whatever, etc. in node.tpl.php.
I am leaning towards #1, especially in light of webchick's reminder above... and the fact that the themers who will be supporting IE6 will probably have to do other hacks anyways, and at that point will probably have modified .tpl.php and .info files all over the place already...
Comment #49
Jeff Burnz CreditAttribution: Jeff Burnz commented@webchick - I can find only one reference to the use of h2 class=title in D7, in function theme_box(), everywhere else it looks like just a strait h2, for which no default styling exists. I think you make a good point that IE6 will be near death sooner rather than much later (yeh gods, lets hope so).
@geerlingguy - 2. would closely mirror page.tpl.php which has id=page-title and class=title, which feels unnecessary. I'm kinda failing to see the point in adding .title at all since you can just reset h2 and we get to cut some fat.
But then again, that's just me, all or nothing;)
Comment #50
EclipseGc CreditAttribution: EclipseGc commented.title would work to our advantage in that blocks, comments and nodes may not all end up with the same headers (i.e. h2 vs h3). I'm still debating this though. jmburz you've made a good argument, I think our question now is "do it right and screw ie6"? or support ie6 for one more iteration. Seeing as how this is stark's code, I'm inclined to say "screw it" but I think that requires a bit more mulling over.
We worked hard on the page tpl css to make it work everywhere... so I just don't know. Let's talk more tomorrow.
Eclipse
Comment #51
sunGuys. IE6 still ranks 29.65% on one of our own, mostly developer-centric websites as of today. IE6 was considered dead from a developer's perspective when Drupal 5 was released. I'm not sure from where you take the prediction that everyone will suddenly start to update to more recent versions in the upcoming 12 months (at maximum). We all want it to die, but it still survives. We cannot change this fact, because it simply is not in our hands.
But, yes, as mentioned before, we can ignore the reality, or accept it. It boils down to: helping themers - or helping standards, screwing away themers.
If my vote would count, I'd say that we can dismiss IE6 when it is no longer used. Equalling the 0.47% of IE5.5 and 0.52% of IE5.0, which are already dismissed today.
Comment #52
EclipseGc CreditAttribution: EclipseGc commentedyou know what the worst part of this whole issue is? IE5.x for mac support direct children selectors...
@sun
I'm not saying we ignore our designer brethren... I happen to count myself amongst their number. What I am saying is that any system that's significantly complicated enough to support IE6 as well as all the other browsers might not be a "sane default" for our markup. And I just think that deserves a night of contemplating.
Comment #53
EclipseGc CreditAttribution: EclipseGc commentedSo in keeping with this issue (which I feel is blocking on an ongoing basis and we should just make a decision) I've placed up a poll on d.o that will run for a week
http://groups.drupal.org/node/20654
Please folks, go vote and let's get some consensus on this.
Eclipse
Comment #54
skilip CreditAttribution: skilip commentedSubscribe
Comment #55
geerlingguy CreditAttribution: geerlingguy commentedTo sum up this thread so far (it's kind of in a freeze until #306358: Add $classes to templates with new theme_process_hook functions is committed):
.user-picture
around$picture
innode.tpl.php
, and (with webchick's approval) replace the.picture
class inuser-picture.tpl.php
with.user-picture
.Edit: Looks like this might be up for inclusion in the node.tpl.php all-encompassing patch: #445530: Remove title attribute from Node titles.
Comment #56
mgiffordD7 isn't going to be really actively used by the majority of the community until the fall of 2010 at the earliest (by my guess). So when thinking about if we can Drop IE6, it's not about what usage it does have now, but what it will have in over a year's time.
I think it can be dropped from core as long as there is an easy way to add back in that functionality into the themes (where it belongs).
Comment #57
jainrutgers CreditAttribution: jainrutgers commentedsubscribe
Comment #58
geerlingguy CreditAttribution: geerlingguy commentedHooray! Now that #306358: Add $classes to templates with new theme_process_hook functions is committed, we can move on this again. I won't have time today or tomorrow to create a new patch, but maybe someone else can? The patch should incorporate the changes outlined in #55, as well as changing the code around the node classes.
Comment #59
Jeff Burnz CreditAttribution: Jeff Burnz commentedTaking in #55, no changes to node classes html, looks like #306358 nailed that.
As I understand it this is where we got up to.
Comment #60
Dries CreditAttribution: Dries commentedThis looks good to me. Feel free to RTBC if you agree.
Comment #61
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis could be a blocker #455844: Allow more granular theming of drupal_render'ed elements
Although if there's any change we'll be re-rolling everything.
Comment #62
webchickLet's try and get this in before #455844. That issue still needs a bit more architectural discussion.
Comment #63
geerlingguy CreditAttribution: geerlingguy commentedCould you also replace the
.picture
class in user-picture.tpl.php with.user-picture
, as was suggested in #55? With that change, I would mark this RTBC, and rejoice at the completion of this issue :-)Comment #64
Jeff Burnz CreditAttribution: Jeff Burnz commentedHere's with user-picture class change rolled in, nails all the class name changes for Garland and user.css + RTL stuff.
Double check I haven't cocked up somewhere, otherwise its good to go.
Comment #65
sunThere is trailing white-space on lines that should be blank.
Comment #66
Jeff Burnz CreditAttribution: Jeff Burnz commentedHmmm, little buggers creeping in :/
Comment #67
webchickDoes it make sense to change the variable to $user_picture or is that just splitting hairs at this point?
Comment #68
EclipseGc CreditAttribution: EclipseGc commentedI think it makes perfect sense to change it to $user_picture at this point... $picture was always sort of "what?" imo anyway.
There are a lot of things here which I think need some tweaking, standards issues (php shouldn't be tabbed in) some missing if statements (specifically the meta div though links could benefit as well). I'll hope in and take a crack at this today. Also I'm a little worried about the default content of $classes, but I'm catching up on this thread so ignore me on that one till I say it again. :-)
Glad to see this moving again.
Eclipse
Comment #69
Jeff Burnz CreditAttribution: Jeff Burnz commentedEclipse, heres one with the if's round the meta div and links, shouldn't have missed that... doh... tabs, hmmm, weird, wondering if eclipse is converting 2 spaces to tabs when generating the patch? odd.
Take it from here, +1 to $user_picture
Comment #70
EclipseGc CreditAttribution: EclipseGc commentedThat's looking MUCH better other than the lack of $user_picture. Get that running and I'll give this a run through.
I looked at the $classes stuff and am very pleased with it. Our next big issue is going to be blocks, and with a few changes that have been made our nodes (well $content in general) is now within a block, so we'll work through that on a separate issue, but I think $node is almost done other than $user_picture.
Good work.
Eclipse
Comment #71
EclipseGc CreditAttribution: EclipseGc commentedChanging status for the $user_picture thing.
Comment #72
geerlingguy CreditAttribution: geerlingguy commentedHaven't looked into this issue for a few weeks; but I think we should get it wrapped so we can move on to the other .tpl files. I'll roll a patch later tonight if I can. Just have to get CVS running on this Mac (ah the joys of using three separate computers...).
Comment #73
geerlingguy CreditAttribution: geerlingguy commentedGaaa!! When did all the .tpl.php files get moved? It's a cool decision, to be sure, but it wreaks havoc on patching! I guess I'll be re-rolling the patch entirely.
[Edit: er... oopsie. I was patching against Drupal 5. Silly me. Back to work!]
Comment #74
geerlingguy CreditAttribution: geerlingguy commentedPlease review - I tested it locally, passed all the pertinent tests. Changed $picture to $user_picture and updated the logic in user.module to pass the correct variable everywhere. (Also note that between the past patch (in #69) and now, some of the logic inside of node.tpl.php changed. I wish I could find where the reason for that is, but alas, I've been out of the loop for a few weeks).
I'm happy with it... are you? Mark as RTBC if you don't find any problem...
Comment #76
geerlingguy CreditAttribution: geerlingguy commentedShucks... due to #455844: Allow more granular theming of drupal_render'ed elements, there's a little added PHP logic in this template. Not an insanely huge deal, but it's there. I can't get the
if
statement to work for the.meta
div; does anyone with more PHP skill know how to fix that? Is this a problem? I'd rather not have the.meta
div show up, personally, if there isn't anything inside!Please see the patch in the next comment for more review.
Comment #77
geerlingguy CreditAttribution: geerlingguy commentedRe-roll after some discussion in #drupal.
Changed node.tpl.php's available variable comments (at the top) to reflect the fact that $picture comes from user-picture.tpl.php, and not from a Drupal 4.x hook, theme_user_picture()
I updated user-picture.tpl.php and user.module (to reflect the change in user-picture.tpl.php), but the variable in the node template is still set to $picture... is this a problem?
Comment #78
geerlingguy CreditAttribution: geerlingguy commentedSorry about all the patches, but I realized I left out a
:
at the end of theif
statement:<?php if ($submitted || !empty($content['links']['terms'])): ?>
Should be pretty much final now... please review!
Comment #79
moshe weitzman CreditAttribution: moshe weitzman commentedI'm pretty sure that this is not needed. You can pass NULL to render(). If you get a NOTICE, we should make render() more robust, not clutter the templates. Lets also remove the check around $submitted.
Comment #80
geerlingguy CreditAttribution: geerlingguy commented@ moshe - I'll change that logic.
But about the submit: not all sites have the submitted enabled, and it's not enabled by default for all content types (I think you can turn that on or off in the theme settings page, no?). And if that's the case, then I'd like to leave the logic in, so an extra
.submitted
div isn't printed on nodes without any submitted information...Also, re: the two help notes at the top for $links and $comments... should those be removed?
Comment #81
geerlingguy CreditAttribution: geerlingguy commentedUpdated patch - removed conditional php for
$content['links']
.Please read the two posts above, and determine whether:
if
logic around$submitted
.$comments
and$links
.Comment #83
geerlingguy CreditAttribution: geerlingguy commentedIs the bot still borked? It worked a couple days ago...
Comment #84
geerlingguy CreditAttribution: geerlingguy commentedJust pinging this thread... can people please review the patch in #81, and tell me your thoughts on the two questions? If we can finish off this template, we can move on to others, and then maybe have all the .tpl.php files in line with a better strategic theming vision.
Comment #85
stBorchertIf you put
<span class="submitted">
intotheme_node_submitted()
you could get rid of theif
logic around$submitted
.And I would replace
$picture
by$user_picture
in node.tpl.php (as done in user-picture.tpl.php). Its kind of confusing to have the user picture available in a variable called "picture".Comment #86
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'd rather not have to override a theme function just to change the HTML for $submitted, its better for themer/designers if its in the template. I can test later this week, sorry flat out at the moment.
Comment #87
stBorchertDo we want to make it a template then (submitted.tpl.php; similar to user-picture.tpl.php)? Or is this overkill?
Comment #88
geerlingguy CreditAttribution: geerlingguy commented@stBorchert - there's actually another issue #364470: Easier theming of "Submitted by author, on date" dealing with the simplification of the $submitted theming... please work on that issue in that thread (and let's not let it hold up this issue—it's almost out of this issue's scope.
Also, with the $picture variable - I tried changing that, and looked in node.module to see if I could get the logic correct to send the variable as $user_picture, but was unsuccessful (I'm not the world's greatest programmer ;-).
Comment #89
stBorchertOk, I'll move to the other thread with the template suggestion.
If this is in you can remove the
if
logic around$submitted
(which will - for now - possibly result in an empy<span>
).The variable
$picture
is defined in theme.inc functiontemplate_preprocess_node
(and in comment.module, profile.module and user.module also).This could be a separate issue ("Change $picture within templates to $user_picture" or something like this).
Comment #90
geerlingguy CreditAttribution: geerlingguy commented@stBorchert - We were given authorization by webchick to rename to user_picture above, so I'll try getting to that and rerolling the patch.
Comment #91
joshmillerJust went over the code for consistency.
Multiple selectors should each be on a single line, with no space after each comma. Checked the css with drupal coding standards: http://drupal.org/node/302199
node.tpl.php is the only .tpl where "$picture" isn't "$user_picture"
Josh
Comment #92
joshmillerComment #93
geerlingguy CreditAttribution: geerlingguy commentedI'll work on this later today, if possible, and submit a new patch.
Comment #94
joshmillerJust found out that core standards for
<?php print $var; ?>
should include the semi-colon, which does not seem to be in any of the most recent patch. Not sure if this is a new syntax for Drupal 7. Doesn't seem to be very "core-like" if it is. For instance, you don't need that extra comma in an array() statement and you don't need to leave off the closing php tag ?> from every page, but we do in the name of preventing mistakes and making sure the code is extremely durable. Keeping the semi-colons in the node.tpl.php for all variables seems to be a very good idea.From http://drupal.org/coding-standards#semicolon
Josh
Comment #95
geerlingguy CreditAttribution: geerlingguy commented@ johhmiller / #94 - I have added in the semicolons. Should be compliant now.
@ joshmiller / #92 - Webchick says we can keep the selector separation thing for after code freeze... and I'm inclined to move the $picture vs. $user_picture thing to a new issue, because I don't want that to hold up the .tpl.php refresh. (as stBorchert recommended in #89)
Other notes: Should apply against HEAD again... we'll see how this thing works out after the test bot.
Comment #96
geerlingguy CreditAttribution: geerlingguy commentedBikeshed issues:
#364470: Easier theming of "Submitted by author, on date"
#538380: Change the $picture variable to $user_picture in node.tpl.php
Comment #98
geerlingguy CreditAttribution: geerlingguy commentedOkay, okay... this patch should work better.
Comment #99
geerlingguy CreditAttribution: geerlingguy commented(Hopefully) final patch - fixed a couple small bugs, and ambiguous language in the docs up top on node.tpl.php.
A summary of what this patch does:
In line with the other tpl-refresh issues, the main goal is to supply themers and designers with intelligent and concise CSS selectors and other material for making lean, mean, and beautiful themes.
Some examples:
picture
selector for nodes touser-picture
in user-picture.tpl.php -picture
is much too generic, and could apply to pretty much any picture in a node.if
statements - added a couple, reworked a couple - the if statements make it so that selectors won't show in empty div's if there's no content (if the empty div's do render, then the styling will be applied, and can cause pages to look quirky).The main goal of this and all the other tpl-refresh issues is to make it so someone can, if they would like, make a beautiful Drupal theme using nothing but a standard Drupal install, a .info file, and a style.css file. Lightweight themes such as Stark are much easier to make if you have sensible default CSS selectors.
Bikeshed issues (need to be worked on to completely finish off issues brought up in this thread):
#364470: Easier theming of "Submitted by author, on date"
#538380: Change the $picture variable to $user_picture in node.tpl.php
We'll also need to move on to other .tpl.php issues and hopefully get the most important ones (like comment.tpl.php) cleaned up before code freeze :-)
Comment #100
geerlingguy CreditAttribution: geerlingguy commentedFirst attempt at including the changing of the $picture variable to $user-picture here... as per webchick's request in IRC :-)
[Edit: Hrm... getting an 'undefined user_picture' warning now... can't work on this anymore tonight. Someone please review the changes in user.module (outlined here: http://drupalbin.com/10779) and see what I'm not getting right.]
Comment #102
JohnAlbinI see the issue. You've updated template_preprocess_user_picture() to use $variables['user_picture'], but you didn't do the same for template_preprocess_node().
Comment #103
geerlingguy CreditAttribution: geerlingguy commentedUpdated node.module, hopefully testbot will be happy this time around...
Comment #105
geerlingguy CreditAttribution: geerlingguy commentedSorry about that; the problem of accidentally triple-clicking the save button! (my hand is a little wired, at this time of morning).
Comment #106
geerlingguy CreditAttribution: geerlingguy commentedWell, here's the actual patch - something went haywire in the system when I clicked so many times (truly, sincerely sorry about that).
Comment #107
stBorchertSome notes:
* node.tpl.php
+ * - $user_picture: The node author's picture.
could be
+ * - $user_picture: The node author's picture. (@see user-picture.tpl.php)
* style-rtl.css
+.user-picture, .comment .submitted {
better do
The class "picture" is still used elsewhere so we should'nt remove it here.
* style.css
You forgot ".user-picture".
Unfortunately I couldn't test the patch here but from the code side it looks good to me (except for the notes above).
Comment #108
geerlingguy CreditAttribution: geerlingguy commented@ stBorchert - ah, thanks for catching those slip-ups! Here's patchy goodness, version 11.
Comment #109
JohnAlbinI found a couple minor issues.
Um… actually, you're referring to profile module and the user.css file. But we should be renaming its $picture and .picture stuff too to be consistent. So I've re-removed .picture and updated profile's files and user.css.
Everything else looks great!
Comment #110
geerlingguy CreditAttribution: geerlingguy commentedI heartily approve (sorry for missing the above items!).
Someday the Garland maintainer might want to take a glance at the .meta .terms section and see if that double-div wrapping is necessary; but that's totally unrelated to this issue.
Comment #111
webchickGREAT work on this everyone!! Committed to HEAD!
At least the $user_picture change needs documenting in the theme upgrade guide. Not sure what else.
So, what's next? :)
Comment #112
stBorchertDo we really do not use
class="picture"
anymore in code? Lets not remove this class from core.Same here. Do not remove
.picture
(only if its not used in core generally).@webchick "So, what's next? :)"
Simplify node.tpl.php after #364470: Easier theming of "Submitted by author, on date" gets in and remove the if-statement around
print $submitted
.This review is powered by Dreditor.
Comment #113
rfayI updated the docs at http://drupal.org/update/theme/6/7#user-picture - If anybody remembers anything else that needs to be documented from this issue, let me know.