WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#24054 closed enhancement (wontfix)

(get_)comment_class() should include a is_user_member_of_blog() class

Reported by: Viper007Bond Owned by: jeremyfelt
Milestone: Priority: normal
Severity: normal Version: 3.5.1
Component: Comments Keywords: has-patch revert
Focuses: multisite Cc:

Description (last modified by Viper007Bond)

On multi-site, there's no way to style comments that are members of the site. You can either style the post author or all registered users (who may not have access to the site's admin area). Example use case would be a multi-author site where you want comments from all authors highlighted.

I think it'd also be cool to include what role they are so that you could style say all authors, editors, and administrators but not style contributors or something like that.

I'm not sure if this would an information exposure issue though (does it matter if it is known what role a user is or that they are a member of the site?).

Attachments (4)

24054_is-member+role.patch (1.5 KB) - added by Viper007Bond 6 years ago.
Adds both "comment-author-is-site-member" and "comment-author-role-ROLE" classes
24054_is-member.patch (1.4 KB) - added by Viper007Bond 6 years ago.
Adds only "comment-author-is-site-member" class incase we don't want to expose roles
24054.diff (1.4 KB) - added by MikeHansenMe 5 years ago.
refreshed from src dir
24054.2.diff (517 bytes) - added by DrewAPicture 4 years ago.

Download all attachments as: .zip

Change History (25)

#1 @Viper007Bond
6 years ago

  • Description modified (diff)

@Viper007Bond
6 years ago

Adds both "comment-author-is-site-member" and "comment-author-role-ROLE" classes

@Viper007Bond
6 years ago

Adds only "comment-author-is-site-member" class incase we don't want to expose roles

#2 @Viper007Bond
6 years ago

  • Keywords has-patch added

#5 @rachelbaker
5 years ago

  • Milestone changed from Awaiting Review to 4.2

Patch looks good. Hope we can get this into 4.2.

@MikeHansenMe
5 years ago

refreshed from src dir

#6 @jeremyfelt
5 years ago

  • Focuses multisite added

I really like comment-author-is-site-member as a built in class to help distinguish network users from site members.

I do think we should leave it up to the comment_class filter to decide if role specific classes should be output as well. I could see not wanting that information displayed.

#7 @jeremyfelt
5 years ago

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

In 31518:

Add comment-author-is-site-member class to comment output for site members.

Add a class to allow targeting of comments made by members of a site rather than users of the entire network.

Props Viper007Bond, MikeHansenMe.

Fixes #24054.

#8 follow-up: @nacin
4 years ago

  • Keywords 2nd-opinion added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I just noticed this. Here are the current class names: byuser, bypostauthor, and now comment-author-is-site-member. Finally, there's comment-author-$login, which means this could hypothetically clash with comment-author. Taken together, we should probably tweak this. I wouldn't mind something like bysiteuser — it's ugly, but at least it is consistent.

I'm not actually sure we should have this. byuser seems enough; a theme probably doesn't need to make a network vs site call all that often. Is it even a theme's call to make? Isn't this better for the comment class filter, and probably to actually hide byuser for non-site users? (Take for example a university where all faculty, staff, and students have an account; the president's blog would probably only have his staff show up as a user.)

#9 @jeremyfelt
4 years ago

bysiteuser is probably better. comment-author-is-site-member is descriptive, but long and very different from existing classes.

I'm still in favor of the addition. There is no distinction between global/network user and site user, so this makes it possible (without filter) to highlight comment authors that are members of a site vs those happen to exist on the same network(s). That said, I don't currently have a use case for this, I can just imagine it coming up. It would be easy to implement the same thing via filter.

#10 in reply to: ↑ 8 ; follow-up: @Viper007Bond
4 years ago

I'm still of the opinion that we need a class to distinguish between global users and local users.

Replying to nacin:

Take for example a university where all faculty, staff, and students have an account; the president's blog would probably only have his staff show up as a user.

That seems really useful and pretty common for any multi-site setup. Why should a theme have to implement it's own functionality for this?

@DrewAPicture
4 years ago

This ticket was mentioned in Slack in #core by drew. View the logs.


4 years ago

#12 @DrewAPicture
4 years ago

  • Keywords commit added; 2nd-opinion removed

24054.2.diff changes the new class name to bysiteuser. I agree there's a valid use case.

#13 in reply to: ↑ 10 @nacin
4 years ago

Replying to Viper007Bond:

That seems really useful and pretty common for any multi-site setup. Why should a theme have to implement it's own functionality for this?

Because the theme doesn't know whether they should be styling based on .byuser or .bysiteuser. It's probably not a decision a theme can or should make.

This ticket was mentioned in Slack in #core by helen. View the logs.


4 years ago

#15 @helen
4 years ago

In 32245:

Comment: rename the comment-author-is-site-member class to bysiteuser for consistency.

Final decision on keeping the class yet to be made.

props DrewAPicture.
see #24054.

This ticket was mentioned in Slack in #core by helen. View the logs.


4 years ago

#17 @DrewAPicture
4 years ago

  • Keywords revert added; commit removed

Consensus from the above-linked scrub was to revert.

#18 @jeremyfelt
4 years ago

In the context of public themes targeting these classes, I agree on a revert.

The use case I'm imagining is likely for networks using a common set of custom themes/plugins that could easily add the class in as needed.

#19 follow-up: @nacin
4 years ago

Also, does is_user_member_of_blog() add significant overhead?

This ticket was mentioned in Slack in #core by helen. View the logs.


4 years ago

#21 in reply to: ↑ 19 @jeremyfelt
4 years ago

Replying to nacin:

Also, does is_user_member_of_blog() add significant overhead?

It may via its use of get_blogs_of_user(), quite a bit happens there—including get_blog_details() for every site a user is a member of. Good catch for caution.

Last edited 4 years ago by jeremyfelt (previous) (diff)

#22 @helen
4 years ago

In 32259:

Don't add a class for comment authors who are members of the current site.

Reverts [32245] and [31518]. We'll keep the whitespace and comment clarification, though.

see #24054.

#23 @helen
4 years ago

  • Milestone 4.2 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.