Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#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's profile SriniG Owned by: lancewillett's profile 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 11 years ago.
23558.diff (1.8 KB) - added by cdog 11 years ago.
23558.png (24.8 KB) - added by cdog 11 years ago.
23558.2.diff (2.4 KB) - added by cdog 11 years ago.
23558.3.diff (2.4 KB) - added by SergeyBiryukov 11 years ago.
Fixed typo in line 388
23558.4.diff (2.4 KB) - added by cdog 11 years ago.
23558.5.diff (1.5 KB) - added by SriniG 11 years ago.
23558.6.diff (2.5 KB) - added by cdog 11 years ago.
23558.5.1.diff (2.2 KB) - added by cdog 11 years ago.

Download all attachments as: .zip

Change History (23)

#1 @lancewillett
11 years ago

  • Milestone changed from Awaiting Review to 3.6

@cdog
11 years ago

@cdog
11 years ago

#2 @cdog
11 years 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.

#3 follow-up: @obenland
11 years 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.

@cdog
11 years ago

#4 in reply to: ↑ 3 @cdog
11 years 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.

@SergeyBiryukov
11 years ago

Fixed typo in line 388

#5 follow-up: @SriniG
11 years 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.

@cdog
11 years ago

#6 in reply to: ↑ 5 ; follow-up: @cdog
11 years 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.

@SriniG
11 years ago

#7 in reply to: ↑ 6 ; follow-up: @SriniG
11 years 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.

@cdog
11 years ago

#8 in reply to: ↑ 7 @cdog
11 years 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?

#9 follow-ups: @obenland
11 years 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.

#10 in reply to: ↑ 9 ; follow-up: @MikeHansenMe
11 years 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.

@cdog
11 years ago

#11 in reply to: ↑ 10 @cdog
11 years ago

Replying to MikeHansenMe:

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

This behavior was intended and, of course, it was only a suggestion.

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

Last edited 11 years ago by cdog (previous) (diff)

#12 in reply to: ↑ 9 @lancewillett
11 years 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.

#13 @lancewillett
11 years ago

  • Priority changed from low to normal

#14 @lancewillett
11 years 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.