WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 5 weeks ago

#50538 assigned enhancement

WP_Comments_List_Table should not show views that have a count of 0

Reported by: pbiron Owned by: pbiron
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-screenshots has-patch
Focuses: administration Cc:

Description

Other core list tables that have a get_views() method do not output a view if the count for that view is 0, e.g., WP_Posts_List_Table doesn't output "Pending (0)" if there are no posts with $post_status === 'pending').
However, WP_Comments_List_Table does output "Pending (0)" if there are no pending comments.

For consistency's sake, I think WP_Comments_List_Table should skip views with count of 0.

Related: #47495

Attachments (4)

Screen Shot 2020-04-21 at 12.38.14 AM.png (41.5 KB) - added by pbiron 5 weeks ago.
50538.diff (1.2 KB) - added by chetan200891 5 weeks ago.
Initial patch.
50538.2.diff (1.1 KB) - added by SergeyBiryukov 5 weeks ago.
50538.3.diff (4.6 KB) - added by pbiron 5 weeks ago.

Download all attachments as: .zip

Change History (18)

#1 @pbiron
5 weeks ago

  • Keywords needs-patch has-screenshots added
  • Owner set to pbiron
  • Status changed from new to assigned

This list table is a little different from others in that not only do it's views are displayed not only on the screen that displays the list table but they are also displayed in the "Recent Activity" dashboard widget. I'll have to check whether making the suggested change would have an adverse impact on the dashboard widget.

#2 follow-up: @SergeyBiryukov
5 weeks ago

I think the process of moderating comments is generally different from working with pending posts, so it might be beneficial to always display "Pending (0)" to make sure there are no more comments left to moderate, even if that's inconsistent with the posts table.

#3 in reply to: ↑ 2 @pbiron
5 weeks ago

Replying to SergeyBiryukov:

I think the process of moderating comments is generally different from working with pending posts, so it might be beneficial to always display "Pending (0)" to make sure there are no more comments left to moderate, even if that's inconsistent with the posts table.

That's similar to what @garret-eclipse said in #47495 in relation to the privacy requests list table, and I don't see the reasoning.

If the Pending view is not displayed then there are no more comments left to moderate. In fact, I would think it would be easier to know that there were none left to moderate if the view were not displayed when the count is 0 (but I don't do comment moderation on real world sites, so maybe those who do have a different perspective).

#4 @SergeyBiryukov
5 weeks ago

The design direction in #47495 was to consistently hide zero-count views on all tables, so this should be good to proceed.

#5 @pbiron
5 weeks ago

OK, I try to get a patch up later today.

#6 follow-up: @pbiron
5 weeks ago

@SergeyBiryukov Can you use your trac searching super powers to try to find out the history of class-wp-comments-list-table.php, L305?

It looks like it's been there for 10+ years...but is very wrong! If I'm reading it correctly, if wp_count_comments() doesn't return a count for some specific status (e.g., because it's been filtered by wp_count_comments), then the count for that status is arbitrarily set to 10...shouldn't it be set to 0 in that case?

that line makes it possible that status views that should be hidden (with the patch I'm working on) won't be...for reasons that are incorrect.

#7 in reply to: ↑ 6 @SergeyBiryukov
5 weeks ago

Replying to pbiron:

It looks like it's been there for 10+ years...but is very wrong! If I'm reading it correctly, if wp_count_comments() doesn't return a count for some specific status (e.g., because it's been filtered by wp_count_comments), then the count for that status is arbitrarily set to 10...shouldn't it be set to 0 in that case?

Good catch, introduced in [9556]. Somehow it never came up before, but yes, that doesn't look right :)

@chetan200891
5 weeks ago

Initial patch.

#8 @chetan200891
5 weeks ago

  • Keywords has-patch added; needs-patch removed

#9 @SergeyBiryukov
5 weeks ago

  • Milestone changed from Awaiting Review to 5.5

#10 @pbiron
5 weeks ago

Hi @chetan200891, thanx for the patch. I'm working on this right now myself.

For this list table, there is JS (in /wp-admin/js/edit-comment.js) that updates the counts for the views when comments are approved/unapproved/marked as spam/etc. And if the views with initial counts of 0 are simply not output, then that JS ends up making the counts seem inconsistent when comments approved/unapproved/etc :-(

I close to having the JS (and 1 change in /wp-admin/css/common.css) updated to work correctly...we'll see if I can get it completely working :-)

#11 @SergeyBiryukov
5 weeks ago

50538.2.diff is just a simplified version of 50538.diff, still doesn't touch the JS code.

@pbiron
5 weeks ago

#12 @pbiron
5 weeks ago

50538.3.diff hides views with count of 0, in such a way that /wp-admin/js/edit-comments.js can show them as comments are approved/unapproved/etc.

This solution is very much a kludge, and may not be worth it. Looking for opinions on that.

  1. instead of simply not outputting 0 count views in PHP (like other list tables), it adds a hidden class to their li elements, along with CSS to hide them
  2. modifies the JS to show/hide the appropriate views when it updates their counts
    • this is complicated by the fact that the "separators" between views are done with the text content in the li elements (instead of via CSS as I think core should do it)
    • because of that, it first wraps the "separators" in a span element, so that it can be hidden for the last "visible" view
    • one consequence of this is that there is a split second when the screen first loads, that the separator for the last "visible" view is displayed...and then it is hidden

I've tested this pretty thoroughly on both the Comments and Dashboard screens, but more testing is certainly warranted.

Also, because of the kludgy nature of the solution, the the selectors (in both the CSS and JS) are more qualified that I would normally do, but I didn't want them to unintentionally affect anything else (e.g., a dashboard widget added by a plugin that just happened to use <ul class="subsubsub">).

Last edited 5 weeks ago by pbiron (previous) (diff)

#13 @SergeyBiryukov
5 weeks ago

  • Milestone changed from 5.5 to 5.6

Since this isn't as straightforward as it looked at a glance, moving to 5.6 to allow for more testing.

#14 @pbiron
5 weeks ago

Replying to SergeyBiryukov:

Since this isn't as straightforward as it looked at a glance, moving to 5.6 to allow for more testing.

Good call. I was going to refresh the patch after [48348], but will hold off on that for now.

Note: See TracTickets for help on using tickets.