Twenty Eleven code review & consistancy

[dd32]

I've just done a review of the Twenty Eleven code looking for anything out of place or inconsistent with other pages.

attached is a diff of problems which I'm going to walk through:

• archive.php - doesnt need to call the_post(); only uses global query conditionals.
• author.php - comment typo
• comments.php - has to check get_option( 'page_comments' ) ? Shouldn't that be included within get_comment_pages_count() instead to short-circuit it? See #17778
• content-aside.php, content-link.php, content-image.php, content-quote.php, - use get_post_type() instead of $post->post_type
• content-gallery.php - indentation (trivial I know)
• content-image.php & a few others - Doesnt use <span class="sep"> | </span> between utility entries?
• content-single.php doesnt have a post = post_type conditional on date meta like most of the rest of the content items.
• content-status.php - Dead code, and avoid using the post type itself
• content.php - Applies to most files, using 'echo=0' on a few items, $show_sep code is rather convuluted as well, perhaps appending to an array and imploding it would be cleaner, This style of code is used in a few places, but not all. $show_sep is stuck there for the time being, array+imploding can't be done with all of the meta's due to echo-only items. Strcuture/indentation could clean it up
• image.php - Has verbose Comments vs. Trackbacks being closed text, This text isnt used on other pages, only mention of Trackbacks are on this template too
• inc/theme-options.php - Use $hook_suffix within an hook name instead of attaching to the generic action
• single.php() - Dead code

Most of this really just needs a sanity check on what is supposed to apply, and apply it to the rest of the template files.

[dd32]

• In the loop, the 'Status' post format doesn't have any title output (only the content). In the Ephemera widget, Only the title is output. In the Single view, The title and content is shown.
• The Status post format has a different layout in the loop (As expected, content-status.php w/ gravatar) however, when the single is viewed, this template is not used. This is because single.php loads content-single.php instead.

[dd32]

• The Aside Post Type doesn't display a Title in the loop, however, It does on it's singular view. Once again, this is because it uses content-single.php

[dd32]

• showcase.php - ​Requeries $featured_args for no apparent reason
• showcase.php - uses a counter to output a list, could just do an if and then a while. Also leaves a </ol> stranded if there are no posts.. which is unlikely
• showcase.php - s/<b>/<strong>/g ? (in the Commenting strings)
• showcase.php again - $xx = new WP_Query(); $xx->query($args); => $xx = new WP_Query( $args );

I'm not coding everything as I run across it to avoid stomping on feet and leaving a huge patch on someones morning email, I'm happy to deal with most of the changes here if someone would like to confirm the expected behaviour on most of the items. Unless there are any objections, I'll work on a patch for cleaning up the PHP tomorrow (ie. 12hours from now) which is separate from the consistency issues outlined above.

[azaozz]

In [18253]:

Twenty Eleven code review and cleanup, props dd32, fixes #17748

[nacin]

As much as I'd like to leave in Y U NO ARRAY in the codebase, there are lots of code comments in there that need addressing.

And I think Twenty Eleven needs a further, final code review by a few of us. I've hit theme-options.php, but neither functions.php nor the template files.

As suggested in IRC, this ticket should ride past RC1, as we're only touching the Twenty Eleven codebase at that point.

[azaozz]

Didn't mean to close the ticket. Should have been "See #...".

[dd32]

Ah yeah.. that patch wasn't designed to be committed.. rather just a tag of where I spotted things during the first comb over. My form of proof reading is to make marks all over the book, check which ones are valid afterwards, then go through making sure everything conforms to it, so many things in there were just inconsistencies.

[dd32]

In [18266]:

Remove debug comments & more housecleaning. See #17748

[azaozz]

Replying to dd32:

...so many things in there were just inconsistencies.

Yes, imho worthy to commit :)

[dd32]

Yes, imho worthy to commit :)

true, eitherway, I'm working through the list and reviewing each file again..

• Comments form submit button is overlapping the grey comment box, Varies between browsers, but most place it outside of the grey area, Looks like it's supposed to be anchored to the corner of the box?

[jorbin]

I went through functions.php and updated some documentation and combined the two body class functions into one.

[nacin]

Looks good at a glance. Feature Image -> Featured Image, also.

[jorbin]

Only one spelling mistake? I've done worse.

[nacin]

That one was pre-existing. I didn't evaluate it for yours yet, but I'd put the over/under at greater than one.

[dd32]

In [18269]:

Showcase template cleanup. See #17748

[dd32]

In [18270]:

Remove Comments/Trackbacks are closed text from image.php template, rely upon the comments closed text within comments.php. Remove duplicated Edit link. See #17748

[dd32]

• content-image.php - Post format Image when viewing in archives is.. different
  • Doesnt display the actual image PECAK
  • The footer meta is a strange colour not inline with the theme
  • The footer meta text is not full width, Categories and Tags end up on different lines
  • Link colour is overridden
• regarding the get_post_type() calls, content-{post format}.php should not need these, Post formats only apply to posts..

[dd32]

In [18272]:

Twenty Eleven Functions.php comments cleanup, merge the 2 body class functions. Props jorbin. See #17748

[dd32]

Custom post types get a weird text on the singular view as well: "This entry was posted in by admin. Bookmark the permalink.", ie. missing category, since, well, they don't have categories.

[dd32]

In [18274]:

Remove the few post_type == post checks on Post Format templates, Post formats only apply to posts. See #17748

[dd32]

In [18285]:

Highlight the current menu item and ancestors, not current menu item and children. See #17748

[kawauso]

Replying to dd32:

In [18274]:

Remove the few post_type == post checks on Post Format templates, Post formats only apply to posts. See #17748

Post format support can be declared for CPTs via ​add_post_type_support() or the 'supports' argument

[dd32]

Post format support can be declared for CPTs via add_post_type_support() or the 'supports' argument

True, Through PHP you can make WordPress do anything, Post formats were originally designed to be Post formats. Eitherway, I feel that changeset is still appropriate, as it's removing old code from when the generic template (used for pages as well) was copied into the format specific templates.

[dd32]

Summery of pending changes here:

1. Coding - Theme has to check get_option('page_comments') in order to determine if comment paging is enabled as get_comment_pages_count() returns > 1 when paging is disabled. See #17778
2. String change - s/<b>/<strong>/g ? comment:3
3. String change - CPT & Categories missing from their "posted in" line, comment:20
4. UI - Image Post Format footer style, comment:18 features a different style and colour: ​2011--footercolourstyle.jpg
   • Background Colour
   • Date in the footer for the image, Posted In & Leave reply have a separator on other types, here, it's a new line, Doesnt use the | Separator which other meta displays use.
   • Has Different Link colour in order to fit in with background colour
   • Edit link can probably float on the same line even
5. UI - Status/Aside title vs. content mis-matches in different places comment:1 comment:2
6. CSS/UI - Comment reply button is not aligned consistantly, comment:11 ​2011-commentreplymisaligned.jpg

If other devs can weigh in here on their expected/preferred outcome on those points, it'd be appreciated as I'm not too fussed about which way they all go, The UI ones just don't feel right to me, but that's not my area of expertise :) (Just ship it! isn't what i'd like to hear either ;))

[dd32]

Another one:

7. UI / String - Edit link, (Edit) for pingbacks, [Edit] for Comments, and Edit (with a grey rounded rectangle background) for Posts/Attachments and CPT's
   • Merge strings? Go for all Edit, or just merge the Comment/Pingback ones to [Edit]
   • If merge all, leaves us with a styling issue, I vote to style them the same as Post Edit links and float to the upper right of the comment. See ​2011-comments.jpg
   • The edit link in archive views is in the Lower left, In singular views it's in the Upper Right.
   • The edit link for Pages is a plain link at the bottom of the content, rather than the grey rectangle in the upper left.
8. Child comments have the Author on it's own line, See ​2011-comments.jpg again for that, Single line leaves whitespace however.

[dd32]

attachment 17748.diff added

• my take on the above points. Doesn't deal with point 1 & 5. Only does parts of 4
  • 1: Can be dealt with in 3.3
  • 4: Changed the Background colour, Havn't modified the style as it's a design choice
  • 5: It's a design choice
  • 7: Edit link styles are consistent, aside from locations (Upper Left in singular views, Upper Right for Comments, Lower Right for Archive views)

[szadok]

dd32: I would love helping you fixing some issues here. Please let me know where can I help. Thanks :)

[zeo]

Please someone review/approve/reject/commit 17748.diff.

[szadok]

I have just downloaded 17748.diff and will test it tomorrow.
Planned to review it earlier.. but had a lot going on my work.
One issue I found on the patch (could be me):
I have applied patch -p0 < 17748.diff
and got:
patching file wp-content/themes/twentyeleven/content-page.php
Hunk #1 FAILED at 16.
1 out of 1 hunk FAILED -- saving rejects to file wp-content/themes/twentyeleven/content-page.php.rej
Patched content-page.php manually
Will review as soon as I can.

[dd32]

attachment 17748.2.diff added

Refreshed (minimal) patch

• content-page.php: Treat Edit link the same as posts for pages, result is the upper left corner rather than a plain link at the bottom of the page
• content-single.php: New string for custom post types which do not have categories
• functions.php: Don't use [Edit] for comments and (Edit) for Pingbacks, instead, uses Edit like everything else
• style.css/rtl.css:
  • Give the Comment Edit link a background like the Reply Comment and Edit post button, Leaves the edit link in it's current location
  • Tack the submit comment button to bottom corner instead of floating randomly over it ​before patch
  • Switches the Image Post type to using a Grey footer instead of the unique colour, can be skipped if need be. ​before patch

[nacin]

Final patch looks good.

[szadok]

looks good. I think it can be submitted

[lancewillett]

Replying to dd32:

• style.css/rtl.css:
  • Tack the submit comment button to bottom corner instead of floating randomly over it ​before patch

Let's leave it as it was, the design is intentional, not random.

Patch has a few minor CSS syntax issues, so I'm posting a refreshed patch without the submit button position change plus cleanup for trailing spaces and CSS formatting.

[lancewillett]

Refresh of 2nd patch for CSS formatting, trim trailing spaces, spaces to tabs

[nacin]

I imagine the lack of intents on table {} at the start of style.css is not intentional.

[lancewillett]

One more try, better fix for the image format entry-meta link hover color on front page

[lancewillett]

Replying to nacin:

I imagine the lack of intents on table {} at the start of style.css is not intentional.

Nope, will fix it right now.

[lancewillett]

Fixes indentation regression

[ryan]

In [18385]:

twentyeleven code tidy up. Props dd32, lancewillett. fixes #17748