Make WordPress Core

Opened 14 years ago

Closed 13 years ago

#17748 closed defect (bug) (fixed)

Twenty Eleven code review & consistancy

Reported by: dd32's profile dd32 Owned by: azaozz's profile azaozz
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.2
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description (last modified by 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.

Attachments (8)

twentyeleven.patch (15.0 KB) - added by dd32 14 years ago.
18267.diff (4.3 KB) - added by jorbin 14 years ago.
18267.2.diff (4.3 KB) - added by jorbin 14 years ago.
17748.diff (12.7 KB) - added by dd32 13 years ago.
17748.2.diff (5.5 KB) - added by dd32 13 years ago.
17748.3.diff (14.0 KB) - added by lancewillett 13 years ago.
Refresh of 2nd patch for CSS formatting, trim trailing spaces, spaces to tabs
17748.4.diff (14.2 KB) - added by lancewillett 13 years ago.
One more try, better fix for the image format entry-meta link hover color on front page
17748.5.diff (14.3 KB) - added by lancewillett 13 years ago.
Fixes indentation regression

Download all attachments as: .zip

Change History (45)

@dd32
14 years ago

#1 @dd32
14 years ago

  • 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.
Last edited 14 years ago by dd32 (previous) (diff)

#2 @dd32
14 years ago

  • 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

#3 @dd32
14 years ago

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

Last edited 14 years ago by dd32 (previous) (diff)

#4 @azaozz
14 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In [18253]:

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

#5 @nacin
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#6 @azaozz
14 years ago

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

#7 follow-up: @dd32
14 years ago

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.

#8 @dd32
14 years ago

In [18266]:

Remove debug comments & more housecleaning. See #17748

#9 @dd32
14 years ago

  • Description modified (diff)

#10 in reply to: ↑ 7 @azaozz
14 years ago

Replying to dd32:

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

Yes, imho worthy to commit :)

#11 @dd32
14 years ago

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?
Last edited 14 years ago by dd32 (previous) (diff)

@jorbin
14 years ago

#12 @jorbin
14 years ago

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

#13 @nacin
14 years ago

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

@jorbin
14 years ago

#14 @jorbin
14 years ago

Only one spelling mistake? I've done worse.

#15 @nacin
14 years ago

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

#16 @dd32
14 years ago

In [18269]:

Showcase template cleanup. See #17748

#17 @dd32
14 years ago

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

#18 @dd32
14 years ago

  • Description modified (diff)
  • 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..
Last edited 14 years ago by dd32 (previous) (diff)

#19 @dd32
14 years ago

In [18272]:

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

#20 @dd32
14 years ago

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.

#21 follow-up: @dd32
14 years ago

In [18274]:

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

#22 @dd32
13 years ago

In [18285]:

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

#23 in reply to: ↑ 21 @kawauso
13 years ago

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

Last edited 13 years ago by kawauso (previous) (diff)

#24 @dd32
13 years ago

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.

#25 @dd32
13 years ago

Summery of pending changes here:

  • 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
  • String change - s/<b>/<strong>/g ? comment:3
  • String change - CPT & Categories missing from their "posted in" line, comment:20
  • 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
    • Has Different Link colour in order to fit in with background colour
    • Edit link can probably float on the same line even
  • UI - Status/Aside title vs. content mis-matches in different places comment:1 comment:2
  • 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 ;))

Version 0, edited 13 years ago by dd32 (next)

#26 @dd32
13 years ago

  • Description modified (diff)

Another one:

  1. 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.
  2. Child comments have the Author on it's own line, See 2011-comments.jpg again for that, Single line leaves whitespace however.
Last edited 13 years ago by dd32 (previous) (diff)

@dd32
13 years ago

#27 @dd32
13 years ago

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)

#28 @szadok
13 years ago

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

#29 @zeo
13 years ago

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

#30 @szadok
13 years ago

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
13 years ago

#31 follow-ups: @dd32
13 years ago

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
Last edited 13 years ago by dd32 (previous) (diff)

#32 @nacin
13 years ago

  • Keywords has-patch added

Final patch looks good.

#33 in reply to: ↑ 31 @szadok
13 years ago

looks good. I think it can be submitted

#34 in reply to: ↑ 31 @lancewillett
13 years ago

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
13 years ago

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

#35 follow-up: @nacin
13 years ago

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

@lancewillett
13 years ago

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

#36 in reply to: ↑ 35 @lancewillett
13 years ago

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
13 years ago

Fixes indentation regression

#37 @ryan
13 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [18385]:

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

Note: See TracTickets for help on using tickets.