WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 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 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 2 years ago.
23558.diff (1.8 KB) - added by cdog 2 years ago.
23558.png (24.8 KB) - added by cdog 2 years ago.
23558.2.diff (2.4 KB) - added by cdog 2 years ago.
23558.3.diff (2.4 KB) - added by SergeyBiryukov 2 years ago.
Fixed typo in line 388
23558.4.diff (2.4 KB) - added by cdog 2 years ago.
23558.5.diff (1.5 KB) - added by SriniG 2 years ago.
23558.6.diff (2.5 KB) - added by cdog 2 years ago.
23558.5.1.diff (2.2 KB) - added by cdog 2 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 @lancewillett2 years ago

  • Milestone changed from Awaiting Review to 3.6

@cdog2 years ago

@cdog2 years ago

comment:2 @cdog2 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.

comment:3 follow-up: @obenland2 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.

@cdog2 years ago

comment:4 in reply to: ↑ 3 @cdog2 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.

@SergeyBiryukov2 years ago

Fixed typo in line 388

comment:5 follow-up: @SriniG2 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.

@cdog2 years ago

comment:6 in reply to: ↑ 5 ; follow-up: @cdog2 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.

@SriniG2 years ago

comment:7 in reply to: ↑ 6 ; follow-up: @SriniG2 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.

@cdog2 years ago

comment:8 in reply to: ↑ 7 @cdog2 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?

comment:9 follow-ups: @obenland2 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.

comment:10 in reply to: ↑ 9 ; follow-up: @MikeHansenMe2 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.

@cdog2 years ago

comment:11 in reply to: ↑ 10 @cdog2 years 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 2 years ago by cdog (previous) (next) (diff)

comment:12 in reply to: ↑ 9 @lancewillett2 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.

comment:13 @lancewillett2 years ago

  • Priority changed from low to normal

comment:14 @lancewillett2 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.