Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geerlingguy’s picture

node.tpl.php, in case anyone is searching for this thread (i.e. me ;-).

Will look into it this evening.

wretched sinner - saved by grace’s picture

sun’s picture

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

webchick’s picture

subscribe.

geerlingguy’s picture

RE #3/sun: I'm 100% with you there. One area that could use about 2000x more improvement in this area is the links section.

JohnAlbin’s picture

/me wonders why he hasn't already subscribed to this. :-p

yoroy’s picture

Issue tags: +tpl-refresh

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

geerlingguy’s picture

@ yoroy: Indeed, the Read more link issues are already being discussed in #16161: Move Read More link to end of node content

geerlingguy’s picture

Status: Active » Needs review
FileSize
1.81 KB

Here's attempt number 1. Criticism shall commence (constructive, of course)!

Changes (I think these are all...):

  1. Wrap $picture in #node-user-picture selector.
  2. Wrap $picture in an if statement. (Most people don't have user pictures enabled anyways).
  3. Wrap the node title in #node-title
  4. Wrap the meta info in .node-meta
  5. Change .submitted to .node-submitted for the $submitted variable.
  6. Change .terms to .node-terms selector.
  7. Change $content wrapper to .node-content (was .content... which was non-specific).
  8. Wrap links in .node-links selector. (There's often more than one set of links on a page).
  9. Wrap comments in a .comments selector. (There's only one set of comments on a given node, so no need to be specific, right?).

Edit: Gaaah! All that typing for naught... I had to retype this post.

webchick’s picture

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

webchick’s picture

Also, if you have .node-terms, what's the point of .terms-inline? (I realize that was there before; just curious)

geerlingguy’s picture

FileSize
2.21 KB

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

JohnAlbin’s picture

Status: Needs review » Needs work

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

geerlingguy’s picture

FileSize
1.75 KB

@JohnAlbin - thanks for the notes. Here's an updated patch:

kika’s picture

+ <div id="node-user-picture">

Again, it's not good to use id attribute on likely repeating element.

geerlingguy’s picture

FileSize
1.77 KB

@kika - D'oh! Thanks for catching that... I was thinking of the node in terms of one page only. Corrected:

Frando’s picture

I would add a class node-{$node->type} to the main node div.

geerlingguy’s picture

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

Frando’s picture

print "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.

sun’s picture

No, 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.:

 <div id="node-<?php print $node->nid; ?>" class="node<?php if ($sticky) { print ' sticky'; } ?><?php if (!$status) { print ' node-unpublished'; } ?> clearfix">

becomes

 <div id="node-<?php print $node->nid; ?>" class="node node-<?php print $node->type; ?> <?php print $status_sticky; ?> <?php print $status_unpublished; ?> clearfix">

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

geerlingguy’s picture

@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

geerlingguy’s picture

Here's the updated patch - no PHP logic included. Should we have the unpublished class be node-unpublished or just unpublished?

geerlingguy’s picture

FileSize
1.97 KB

Whoops! *Here's* the updated patch...

geerlingguy’s picture

FileSize
1.97 KB

Gaa! 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).

yoroy’s picture

Status: Needs work » Needs review

setting status

Status: Needs review » Needs work

The last submitted patch failed testing.

geerlingguy’s picture

Hmm... 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?

geerlingguy’s picture

FileSize
1.96 KB

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

  1. Took off the 'node-' selectors on classes inside the .node class... there is such a thing as too much specificity ;-)
  2. Fixed the problem causing the exceptions in #24's patch - this section of the code prints out the classes for each node, and will need more work. See #306358: Add $classes to templates with new theme_process_hook functions (also referenced in the code of the patch) for more info.

Also: After rolling this patch, it was determined we should take out the CSS class .user-picture around $picture in node.tpl.php, and (with webchick's approval) replace the .picture class in user-picture.tpl.php with .user-picture. These changes will be rolled into the next version of this patch.

sun’s picture

Took off the 'node-' selectors on classes inside the .node class... there is such a thing as too much specificity ;-)

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

geerlingguy’s picture

As long as we (themers) have to support browsers that do not properly support current CSS standards, we need specific CSS classes.

I'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).

EclipseGc’s picture

@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

geerlingguy’s picture

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

sun’s picture

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

.node .title { margin: 2em; }
.block .title { font-weight: bold; }
.node .block .title { margin: 0; }
.block .node .title { font-weight: normal; }

Much cleaner:

.node-title { margin: 2em; }
.block-title { font-weight: bold; }

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.

EclipseGc’s picture

ok, 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.

Jeff Burnz’s picture

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

.node-title,
.block-title {default: values;}

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.

geerlingguy’s picture

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

ejort’s picture

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

Jeff Burnz’s picture

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

<h2 class="node-title title">
you can't even do this without using contrib/custom modules

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.

EclipseGc’s picture

ok, 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

sun’s picture

Sure, 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?

akahn’s picture

Sun, 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.

sun’s picture

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

EclipseGc’s picture

@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

sun’s picture

Yes, 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.

EclipseGc’s picture

ok, 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

Jeff Burnz’s picture

FileSize
7.84 KB

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

webchick’s picture

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

geerlingguy’s picture

RE #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...

Jeff Burnz’s picture

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

EclipseGc’s picture

.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

sun’s picture

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

EclipseGc’s picture

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

EclipseGc’s picture

So 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

skilip’s picture

Subscribe

geerlingguy’s picture

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

  • The latest patch in #28 is not complete; a few little things are missing due to the thread referenced above. Also, a few tweaks will occur, including the removal of the wrapping div of the h2/node title.
  • The next version of this patch will take out the CSS class .user-picture around $picture in node.tpl.php, and (with webchick's approval) replace the .picture class in user-picture.tpl.php with .user-picture.
  • In IRC, sun and EclipseGc came to the conclusion that core/Stark would be clean, standards-compliant markup, but other themes would be IE6 compatible... meaning comments #29-52ish can be read as an aside ;-)
  • Once we get this .tpl.php finished, what's next? Comments? Book pages? etc?

Edit: Looks like this might be up for inclusion in the node.tpl.php all-encompassing patch: #445530: Remove title attribute from Node titles.

mgifford’s picture

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

jainrutgers’s picture

subscribe

geerlingguy’s picture

Hooray! 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.

Jeff Burnz’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

Taking in #55, no changes to node classes html, looks like #306358 nailed that.

As I understand it this is where we got up to.

Dries’s picture

This looks good to me. Feel free to RTBC if you agree.

Jeff Burnz’s picture

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

webchick’s picture

Let's try and get this in before #455844. That issue still needs a bit more architectural discussion.

geerlingguy’s picture

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

Jeff Burnz’s picture

Here'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.

sun’s picture

Status: Needs review » Needs work
+ 

There is trailing white-space on lines that should be blank.

Jeff Burnz’s picture

Status: Needs work » Needs review
FileSize
4.18 KB

Hmmm, little buggers creeping in :/

webchick’s picture

Does it make sense to change the variable to $user_picture or is that just splitting hairs at this point?

EclipseGc’s picture

I 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

Jeff Burnz’s picture

Eclipse, 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

EclipseGc’s picture

That'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

EclipseGc’s picture

Status: Needs review » Needs work

Changing status for the $user_picture thing.

geerlingguy’s picture

Haven'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...).

geerlingguy’s picture

Gaaa!! 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!]

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
6.13 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

geerlingguy’s picture

FileSize
6.18 KB

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

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
6.63 KB

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

geerlingguy’s picture

FileSize
6.72 KB

Sorry about all the patches, but I realized I left out a : at the end of the if statement:
<?php if ($submitted || !empty($content['links']['terms'])): ?>

Should be pretty much final now... please review!

moshe weitzman’s picture

Status: Needs review » Needs work
-  <?php render($content['links']); ?>
+  <?php if ($content['links']): ?>
+    <?php render($content['links']); ?>
+  <?php endif; ?>

I'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.

geerlingguy’s picture

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

 * - $comments: the themed list of comments (if any).
 . . .
 * - $links: Themed links like "Read more", "Add new comment", etc. output
 *   from theme_links().
geerlingguy’s picture

Status: Needs work » Needs review
FileSize
6.51 KB

Updated patch - removed conditional php for $content['links'].

Please read the two posts above, and determine whether:

  1. We should remove the php if logic around $submitted.
  2. We should remove the help notes at the top of node.tpl.php for $comments and $links.

Status: Needs review » Needs work

The last submitted patch failed testing.

geerlingguy’s picture

Status: Needs work » Needs review

Is the bot still borked? It worked a couple days ago...

geerlingguy’s picture

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

stBorchert’s picture

If you put <span class="submitted"> into theme_node_submitted() you could get rid of the if 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".

Jeff Burnz’s picture

I'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.

stBorchert’s picture

Do we want to make it a template then (submitted.tpl.php; similar to user-picture.tpl.php)? Or is this overkill?

geerlingguy’s picture

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

stBorchert’s picture

Ok, 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 function template_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).

geerlingguy’s picture

@stBorchert - We were given authorization by webchick to rename to user_picture above, so I'll try getting to that and rerolling the patch.

joshmiller’s picture

Just went over the code for consistency.

  1. +.user-picture, .comment .submitted {
    

    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

  2. + * - $picture: The author's picture (passed from user-picture.tpl.php).
    
    ...
    
    +  <?php print $picture ?>
    

    node.tpl.php is the only .tpl where "$picture" isn't "$user_picture"

Josh

joshmiller’s picture

Status: Needs review » Needs work
geerlingguy’s picture

I'll work on this later today, if possible, and submit a new patch.

joshmiller’s picture

Just 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

<?php print $title; ?> -- Drupal Coding Standard
<?php print $title ?> -- Current patch (#81) recommended for node.tpl.php
<?=$title?> -- Wouldn't it be nice?

Josh

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
7.71 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
5.63 KB

Okay, okay... this patch should work better.

geerlingguy’s picture

FileSize
5.69 KB

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

  • Changing the picture selector for nodes to user-picture in user-picture.tpl.php - picture is much too generic, and could apply to pretty much any picture in a node.
  • PHP 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 :-)

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
7.97 KB

First 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.]

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs review » Needs work

I 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().

geerlingguy’s picture

FileSize
6.95 KB

Updated node.module, hopefully testbot will be happy this time around...

geerlingguy’s picture

Status: Needs work » Needs review

Sorry about that; the problem of accidentally triple-clicking the save button! (my hand is a little wired, at this time of morning).

geerlingguy’s picture

FileSize
6.95 KB

Well, here's the actual patch - something went haywire in the system when I clicked so many times (truly, sincerely sorry about that).

stBorchert’s picture

Some 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

.picture,
.user-picture, 
.comment .submitted {

The class "picture" is still used elsewhere so we should'nt remove it here.

* style.css

+.picture,
+.comment .submitted {

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

geerlingguy’s picture

FileSize
7.2 KB

@ stBorchert - ah, thanks for catching those slip-ups! Here's patchy goodness, version 11.

JohnAlbin’s picture

I found a couple minor issues.

  1. #108 patch removes newline from end of node.tpl.
  2. We forgot to change the $picture variable in Garland.
  3. We should clean up Garland's node.tpl.php in the same way we are doing for the default.
  4. For some reason, we weren't print render()ing the $content['links']['terms']. A previous cleanup patch must have missed that.
  5. The class "picture" is still used elsewhere so we should'nt remove it here.

    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!

geerlingguy’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

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

stBorchert’s picture

+++ themes/garland/style-rtl.css	6 Aug 2009 03:33:40 -0000
@@ -206,7 +206,8 @@ ul.links li, ul.inline li {
-.picture, .comment .submitted {

Do we really do not use class="picture" anymore in code? Lets not remove this class from core.

+++ themes/garland/style.css	6 Aug 2009 03:33:40 -0000
@@ -674,7 +674,8 @@ ul.links li, ul.inline li {
-.picture, .comment .submitted {

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.

rfay’s picture

Status: Needs work » Fixed

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

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation, -tpl-refresh

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