WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#35392 closed defect (bug) (fixed)

Dashboard "Recent Comments" widget improvements

Reported by: afercia Owned by: afercia
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Widgets Keywords: has-screenshots has-patch
Focuses: ui, accessibility Cc:

Description

While working on #35049 I've noticed there's room for some accessibility and functional improvements for the "Recent Comments" dashboard widget. I'd propose some simple changes, trying to summarize:

Semantics
Since it's a list of comments, should be marked up as a list. Practical benefits: screen readers will announce the number of items ("list with n items") and users will be able to jump through list items using their screen reader dedicated keys (for example in NVDA is "i").

Link to the post
When users have the edit_post capability, the widget displays the title of the commented post with a link to the post (see screenshot below on the left). Otherwise, the title is completely removed (on the right). Notice also the small glitch for Pingbacks where a stray "on" is displayed.

https://cldup.com/XL8til_xa1.png

This seems a bit inconsistent with what seems to me an established convention in the admin. As far as I see, usually the item title is always displayed. It's just the link that gets removed for users without the edit capability. Maybe I'm missing something :) Wondering how a comment without any reference to the post it was submitted on would be of any help, see below. Sure, users can't edit posts but at least they will have some contextual information.

https://cldup.com/YmMrJp_jRO.png

Heading
I'd propose to restore the text "Recent Comments" that was used before the Dashboard redesign, see [26144]. Saying just "Comments" doesn't help users to immediately understand this is just a short list with the latest 5 comments.

Views links
At the bottom of the widget, the links "All, Pending, Approved, etc." are not so clear when read out of context. These links should either be expanded with some aria-label attributes or be preceded by a new heading. To keep it simple I'd propose to add a heading, something like "View more comments".

Pending status
Only the pending comments (not pingbacks or trackbacks) have a small "flag" icon followed by a text string [Pending] to indicate their status. This info is dynamically displayed/hidden when approving or unapproving comments. Was considering to hide all this from screen readers and provide a more meaningful, expanded, text hidden with screen-reader-text. Open to suggestions.
About the flag icon, maybe worth considering some visual improvements too :)

Attachments (5)

35392.patch (5.7 KB) - added by afercia 6 years ago.
35392.2.patch (5.8 KB) - added by afercia 6 years ago.
curly quotes should be translatable
35392.3.diff (5.9 KB) - added by rachelbaker 6 years ago.
Only show "on" if there is a post title to show
35392.4.patch (5.7 KB) - added by afercia 6 years ago.
35392.5.patch (6.1 KB) - added by afercia 6 years ago.

Download all attachments as: .zip

Change History (28)

@afercia
6 years ago

#1 @afercia
6 years ago

  • Keywords has-screenshots has-patch added

First pass. Also, avoids to output an empty paragraph when there are no action links.

#2 follow-up: @melchoyce
6 years ago

Good point about the post titles — I think we should always show the title, and that it should link to the post itself, not the edit screen. It doesn't really make sense in this context, especially when you might be quickly moderating a comment and then want to look at it live. If the title always links to the post itself, then there also aren't any weird permission issues.

I'd propose to restore the text "Recent Comments" that was used before the Dashboard redesign

Agreed.

About the flag icon, maybe worth considering some visual improvements too :)

Definitely — it looks a little awkward.

@afercia
6 years ago

curly quotes should be translatable

#3 in reply to: ↑ 2 @afercia
6 years ago

Replying to melchoyce:

Good point about the post titles — I think we should always show the title, and that it should link to the post itself, not the edit screen. It doesn't really make sense in this context, especially when you might be quickly moderating a comment and then want to look at it live.

Thinking at this, when the comment is approved, it is possible to link to the in-page comment e.g. my-post-title/#comment-3. When the comment is pending it won't appear on the front end so the link should point to the post e.g. my-post-title/.

By the way, some themes and plugins hide the comments in a expandable div or things like that. So maybe keep it simple and always link to the post?

#4 @afercia
6 years ago

  • Owner set to afercia
  • Status changed from new to assigned

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


6 years ago

#6 @ramiy
6 years ago

@afercia,

Replace this code:

printf( 
	/* translators: 1: comment author, 2: post title, 3: notification if the comment is pending */ 
	__( 'From %1$s on %2$s%3$s' ), 
	'<cite class="comment-author">' . get_comment_author_link( $comment ) . '</cite>', 
	$comment_post_link_or_title, 
	' <span class="approve" aria-hidden="true">' . __( '[Pending]' ) . '</span>' 
);

With this one:

printf( 
	/* translators: 1: comment author, 2: post title, 3: notification if the comment is pending */ 
	__( 'From %1$s on %2$s %3$s' ), 
	'<cite class="comment-author">' . get_comment_author_link( $comment ) . '</cite>', 
	$comment_post_link_or_title, 
	'<span class="approve" aria-hidden="true">' . __( '[Pending]' ) . '</span>' 
 );

The change:

  • %2$s%3$s => %2$s %3$s
  • ' <span => '<span

Moving the space before the span, into the string between the two placeholders.

#7 @afercia
6 years ago

@ramiy the patch doesn't touch that part, it's already that way in core. A good, general, rule for patches is to don't touch things that are unrelated to the scope of the ticket. That's also to don't pollute diffs with lots of changed lines. I'm not opposed to small, additional, improvements if I only knew why that's an improvement :) Can you please expand and explain why that's better for translations?

@rachelbaker
6 years ago

Only show "on" if there is a post title to show

#8 @rachelbaker
6 years ago

@afercia While testing your patch, I noticed that if a recent comment didn't have an attached post the text "on" still appeared.

Screenshot:
https://cldup.com/OQrrc9RnvN.png

I added back the conditional check in 35392.3.diff for your consideration.

Updated screenshot:
https://cldup.com/G8hjx8vKdd.png

#9 @afercia
6 years ago

@rachelbaker thanks! Any suggestions about the post link destination? Currently, it points to the Edit screen and it was suggested to link to the post itself.

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


6 years ago

@afercia
6 years ago

#11 @afercia
6 years ago

Refreshed patch. The post link now points to the post itself, not the Edit screen. This also allows to remove the check for permissions. Replying to melchoyce:

About the flag icon, maybe worth considering some visual improvements too :)

Definitely — it looks a little awkward.

The "Pending" status text should be improved a bit but I'd wait for a design proposal and then try to improve it also in the Comments screen.

There's one last thing that needs to be taken care of or, better, needs a decision: should pingbacks, trackbacks and other comment types have the "pending" text and flag?

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

#13 @afercia
6 years ago

Discussed a bit in today's extra meeting on Slack > accessibility and couldn't find a reason why pending pingbacks and trackbacks ​wouldn't need to​ be labeled as pending. They also need some kind of feedback about their status (besides color). We'd suggest to pair them with the pending comments and then maybe improve the visual part (the "flag") and semantics in a future iteration.

@afercia
6 years ago

#14 @afercia
6 years ago

  • Keywords commit added

Refreshed patch to add a pending status indicator to Pingbacks/Trackbacks/other types of comments too. See in the screenshot below. Also, decreases a bit the left margin of the "flag" trying to reduce the cases when it will break in a new line. As agreed, the visual part could be improved a bit in future iterations.

https://cldup.com/IWyv7rKuEE.png

#15 @rachelbaker
6 years ago

Looks great to me @afercia.

#16 @afercia
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 36683:

Accessibility: improve accessibility of the Dashboard "Recent Comments" widget.

  • Makes the list of comments a list
  • Always displays the title of the post the comment relates to, linked to the post itself and no more to the Edit screen
  • Headings: changes the visible one in "Recent Comments" and adds a hidden "View more comments" heading before the views links
  • Adds the pending status indicator to Pingbacks and Trackbacks

Props rachelbaker, afercia.

Fixes #35392.

#17 @ocean90
6 years ago

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

@afercia Take a look at this line: trunk/src/wp-admin/includes/dashboard.php?annotate=blame#L716.
Seems like copy/paste from line 679. Can't we simplify this to <strong>$type</strong> <span class="approve">' . __( '[Pending]' ) . '</span>?

Last edited 6 years ago by ocean90 (previous) (diff)

#18 @afercia
6 years ago

@ocean90 nice link ;) yep, looks like I forgot to add the second placeholder, simplifing makes sense. Thanks.

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


6 years ago

#20 @afercia
6 years ago

Note for posterity: agreed to just add the missing placeholder.

#21 @afercia
6 years ago

  • Keywords 2nd-opinion removed

#22 @afercia
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 36767:

Comments: Add missing placeholder for printf() after [36683].

Fixes #35392.

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


6 years ago

Note: See TracTickets for help on using tickets.