WordPress.org

Make WordPress Core

#23558 closed defect (bug) (fixed)

Twenty Thirteen: Comment author's name looks out of place when avatar display is turned off in discussion settings.

Reported by: SriniG Owned by: lancewillett
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords: has-patch needs-refresh
Focuses: Cc:

Description

Comment author's name looks out of place when 'Avatar Display' is unchecked in Settings->Discussion. The layout looks as if something is missing, this shouldn't be.

Attachments (9)

twentythirteen-comments-without-gravatars.png (32.3 KB) - added by SriniG 14 months ago.
23558.diff (1.8 KB) - added by cdog 14 months ago.
23558.png (24.8 KB) - added by cdog 14 months ago.
23558.2.diff (2.4 KB) - added by cdog 14 months ago.
23558.3.diff (2.4 KB) - added by SergeyBiryukov 14 months ago.
Fixed typo in line 388
23558.4.diff (2.4 KB) - added by cdog 14 months ago.
23558.5.diff (1.5 KB) - added by SriniG 14 months ago.
23558.6.diff (2.5 KB) - added by cdog 14 months ago.
23558.5.1.diff (2.2 KB) - added by cdog 14 months ago.

Download all attachments as: .zip

Change History (23)

comment:1 lancewillett14 months ago

  • Milestone changed from Awaiting Review to 3.6

cdog14 months ago

cdog14 months ago

comment:2 cdog14 months ago

  • Cc catalin.dogaru@… added
  • Keywords has-patch needs-testing added

attachment:23558.diff uses the show_avatars option to check wheter to display avatar images or not. See attachment:23558.png for preview.

comment:3 follow-up: obenland14 months ago

I like the approach of using a comment class. It would be cleaner though, to use either a 'comment_class' callback or the 'body_class' callback we already have in functions.php.
We should be able to just switch to the mobile-style display of comments then.

cdog14 months ago

comment:4 in reply to: ↑ 3 cdog14 months ago

Replying to obenland:

I like the approach of using a comment class. It would be cleaner though, to use either a 'comment_class' callback or the 'body_class' callback we already have in functions.php.

Thanks for your feedback. I've updated the patch to use the comment_class filter hook. Please check: attachment:23558.2.diff.

SergeyBiryukov14 months ago

Fixed typo in line 388

comment:5 follow-up: SriniG14 months ago

@cdog: I don't think we need to conditionally check get_option( 'show_avatar' ) in order to echo get_avatar(). It seems the function get_avatar() somehow handles the condition by itself. If you view source after disabling avatars, you'll find there is no empty img tag or anything like that.

I am also not in favour of introducing a middot, or changing anything inside the twentythirteen_comment function, unless absolutely necessary. All changes can be made via CSS making use of the no-avatar class added through the comment_class filter hook.

cdog14 months ago

comment:6 in reply to: ↑ 5 ; follow-up: cdog14 months ago

Replying to SriniG:

@cdog: I don't think we need to conditionally check get_option( 'show_avatar' ) in order to echo get_avatar(). It seems the function get_avatar() somehow handles the condition by itself. If you view source after disabling avatars, you'll find there is no empty img tag or anything like that.

Agreed. I've updated the patch to use only the get_avatar() function. Please check: attachment:23558.4.diff.

I am also not in favour of introducing a middot, or changing anything inside the twentythirteen_comment function, unless absolutely necessary. All changes can be made via CSS making use of the no-avatar class added through the comment_class filter hook.

I'm not sure if we can get the same result as in attachment:23558.png (excluding the middot) only using CSS in a clean way.

SriniG14 months ago

comment:7 in reply to: ↑ 6 ; follow-up: SriniG14 months ago

@cdog: Please check attachment:23558.5.diff. Of course this just switches to a styling similar to the mobile view as per comment 3 by obenland. If you want to add a middot, you can have that too, you can use the after pseudo selector, like .no-avatars .comment-author:after { content: '\00b7'; }. Also modify the margins and floats if neeeded.

cdog14 months ago

comment:8 in reply to: ↑ 7 cdog14 months ago

Replying to SriniG:

@cdog: Please check attachment:23558.5.diff. Of course this just switches to a styling similar to the mobile view as per comment 3 by obenland.

It's clean and simple. I like it. :)

Personally I prefer the comment author link and comment time on the same line (similar to DISQUS). Please check: attachment:23558.6.diff. What do you think about this approach?

comment:9 follow-ups: obenland14 months ago

  • Keywords commit added; needs-testing removed

Let's go with 23558.5.diff. Joen approved the mobile styles already and I don't really see a reason to introduce a new styling here.

We might even be able to get rid of the .no-avatars .comment-author .fn selector (same goes for the equivalent in the mobile media query) once #23514 makes it in.

comment:10 in reply to: ↑ 9 ; follow-up: MikeHansenMe14 months ago

  • Cc mdhansen@… added

Replying to obenland:

Let's go with 23558.5.diff. Joen approved the mobile styles already and I don't really see a reason to introduce a new styling here.

I agree, when I tested .6 it removes the name below the avatar when they are enabled.

.5 does not seem to have any side effects.

cdog14 months ago

comment:11 in reply to: ↑ 10 cdog14 months ago

Replying to MikeHansenMe:

I agree, when I tested .6 it removes the name below the avatar when they are enabled.

It was only a suggestion and that behavior was intended.

.5 does not seem to have any side effects.

Please check: attachment:23558.5.1.diff. This patch is the same as attachment:23558.5.diff with the following changes/additions:

  • replaced no-avatars with no-avatar for the comment_class callback as a comment contains only one avatar - add this if it's really necessary;
  • added the no-avatars class for the body_class callback as it could be useful for pages containing avatars in other places than comments (e.g. author pages) - this should be sufficient for most cases.

Note: Again, this is just a suggestion. Please feel free to choose the most suitable solution.

Version 2, edited 14 months ago by cdog (previous) (next) (diff)

comment:12 in reply to: ↑ 9 lancewillett14 months ago

  • Keywords needs-refresh added; commit removed
  • Priority changed from normal to low

Replying to obenland:

Let's go with 23558.5.diff. Joen approved the mobile styles already and I don't really see a reason to introduce a new styling here.

Noted.

We might even be able to get rid of the .no-avatars .comment-author .fn selector (same goes for the equivalent in the mobile media query) once #23514 makes it in.

I'd like to wait for #23514, then, before we push this one. Then we can update the patch here with the simpler selectors.

comment:13 lancewillett14 months ago

  • Priority changed from low to normal

comment:14 lancewillett14 months ago

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

In 23498:

Twenty Thirteen: style comments correctly when show_avatars option is turned off in Discussion settings. Props SriniG, fixes #23558.

Note: See TracTickets for help on using tickets.