Opened 13 years ago
Closed 13 years ago
#17748 closed defect (bug) (fixed)
Twenty Eleven code review & consistancy
Reported by: | dd32 | Owned by: | azaozz |
---|---|---|---|
Milestone: | 3.2 | Priority: | normal |
Severity: | normal | Version: | 3.2 |
Component: | Bundled Theme | Keywords: | has-patch |
Focuses: | Cc: |
Description (last modified by )
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 typocomments.php - has to check get_option( 'page_comments' ) ? Shouldn't that be included within get_comment_pages_count() instead to short-circuit it?See #17778content-aside.php, content-link.php, content-image.php, content-quote.php, - use get_post_type() instead of $post->post_typecontent-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 tooinc/theme-options.php - Use $hook_suffix within an hook name instead of attaching to the generic actionsingle.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)
Change History (45)
#1
@
13 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.
#2
@
13 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
@
13 years ago
showcase.php - Requeries $featured_args for no apparent reasonshowcase.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.
#4
@
13 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In [18253]:
#5
@
13 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.
#7
follow-up:
↓ 10
@
13 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.
#10
in reply to:
↑ 7
@
13 years ago
Replying to dd32:
...so many things in there were just inconsistencies.
Yes, imho worthy to commit :)
#11
@
13 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 anshored to the corner of the box?
#12
@
13 years ago
I went through functions.php and updated some documentation and combined the two body class functions into one.
#15
@
13 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.
#18
@
13 years ago
- Description modified (diff)
- content-image.php - Post format Image when viewing in archives is.. different
Doesnt display the actual imagePECAK- 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..
#20
@
13 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.
#23
in reply to:
↑ 21
@
13 years ago
Replying to dd32:
In [18274]:
Post format support can be declared for CPTs via add_post_type_support() or the 'supports' argument
#24
@
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
@
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, 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
- 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 ;))
#26
@
13 years ago
- Description modified (diff)
Another one:
- 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.
- Child comments have the Author on it's own line, See 2011-comments.jpg again for that, Single line leaves whitespace however.
#27
@
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
@
13 years ago
dd32: I would love helping you fixing some issues here. Please let me know where can I help. Thanks :)
#30
@
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.
#31
follow-ups:
↓ 33
↓ 34
@
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
#34
in reply to:
↑ 31
@
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.
#35
follow-up:
↓ 36
@
13 years ago
I imagine the lack of intents on table {} at the start of style.css is not intentional.
@
13 years ago
One more try, better fix for the image format entry-meta link hover color on front page