Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#43587 closed enhancement (fixed)

UI adjustments to the Status box in the Edit Comment screen

Reported by: birgire's profile birgire Owned by: nfmohit's profile nfmohit
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-screenshots good-first-bug has-patch commit
Focuses: ui, administration Cc:

Description

Related to #43586

Also few suggestions for the Status box in the Edit Comment screen:

  • Add an underline on the box title (like in meta-box headers).
  • Change the box title from Status to Save.
  • Use the familiar layout from the post Publish meta-box.
  • Use the status layout from the post Publish meta-box. But maybe it's too many clicks?
  • Use the status format icon (don't know if it's only used for post status?)
  • Use the comments icon for the In response to row.

Location example: /wp-admin/comment.php?action=editcomment&c=123

Attachments (13)

before.jpg (17.4 KB) - added by birgire 6 years ago.
after.jpg (16.8 KB) - added by birgire 6 years ago.
edit-media-save-box.jpg (21.7 KB) - added by birgire 6 years ago.
43587.diff (6.6 KB) - added by nfmohit 6 years ago.
43587.2.diff (6.5 KB) - added by nfmohit 6 years ago.
Changes regarding coding standards
save-box-with-left-margin-on-form-inputs.jpg (21.5 KB) - added by birgire 6 years ago.
43587.3.diff (4.4 KB) - added by nfmohit 6 years ago.
Removes the comment status 'Edit' toggle and contains slight visual improvements and corrections
43587-4.diff (3.9 KB) - added by birgire 5 years ago.
from-43587-4.diff.jpg (19.4 KB) - added by birgire 5 years ago.
edit-mode-from-43587-4.diff.jpg (21.8 KB) - added by birgire 5 years ago.
before-and-after.jpg (26.5 KB) - added by birgire 5 years ago.
43587-5.diff (3.8 KB) - added by birgire 5 years ago.
43587-6.diff (3.7 KB) - added by birgire 4 years ago.

Download all attachments as: .zip

Change History (51)

@birgire
6 years ago

@birgire
6 years ago

#1 @melchoyce
6 years ago

  • Keywords needs-patch has-screenshots good-first-bug added

It seems ideal here to wholesale steal styles, or update the markup, to match the Editor status box.

#2 @afercia
6 years ago

I'd agree the visual proposed changes increase consistency and that's very welcome. However, I'd suggest to better consider a couple things:

  • title: I'm not sure "Save" would be appropriate, since a comment can't be "saved". The actual available actions are related to changing the status and the date.
  • I'd keep the 3 radio buttons, also for accessibility reasons. I'm not sure making the interaction more complicated, especially for keyboard and assistive technologies users, is a good option just for the sake of better visuals. I'd rather consider to keep it simple, as it is know.

#3 @birgire
6 years ago

@afercia thanks for your feedback.

  • Regarding the box title, I looked at the Edit Media screen and noticed it uses Save as the title. Attached the screenshot edit-media-save-box.jpg. But as we can in fact also modify other fields on the Edit Comment screen, like name, email, url and content, apart from the status and date, I thought Save would also make sense in this case. English is not my first language, so I guess there might be other better titles available ;-)
  • Regarding the status radio buttons, I agree. I now wonder if the post status settings on the Post Edit screen is sub-optimal regarding accessibility?

#4 @afercia
6 years ago

Ah right, I haven't considered the other fields! (English is not my native language too ;))

I now wonder if the post status settings on the Post Edit screen is sub-optimal regarding accessibility?

Well it is.. complicated :) We've tried to improve it but it's not possible to change it drastically, unless there's a complete re-design of the whole box. With the advent of Gutenberg, it will also be used less frequently.

#5 @birgire
6 years ago

Well it is.. complicated :) We've tried to improve it but it's not possible to change it drastically, > unless there's a complete re-design of the whole box. With the advent of Gutenberg, it will also be used less frequently.

hehe, probably better not to touch that ;-)

ps: Not related to this ticket, but I find it confusing that when we save a comment as "Approved" we are redirected to the front-end, otherwise if we save it as "Spam" or "Pending" we are redirected to the comments list table screen. So I feel a little sea-sick after playing with the Edit Comment screen ;-)

#6 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.0

@nfmohit
6 years ago

#7 @nfmohit
6 years ago

  • Keywords has-patch added; needs-patch removed

The 435687.diff addresses all the enhancements mentioned in this ticket.

@nfmohit
6 years ago

Changes regarding coding standards

#8 @nfmohit
6 years ago

Please consider reviewing 43587.2.diff over 43587.diff as it contains slight changes regarding coding standards.

#9 @birgire
6 years ago

Thanks for the patch @nfmohit

The patch 43587.2.diff seems to work as expected to the original proposal.

@afercia suggested here (#2) that we should display the radio buttons by default and not use the extra "Edit" link to display it, for accessibility reasons.

I attached another screenshot in save-box-with-left-margin-on-form-inputs.jpg as a suggestion for that.

On that screenshot I also added an extra left margin on the holders of the form inputs. What do you think? (I opened the timestamp edit, just to better show the alignment there)

Do we need the bold status text, shown in the screenshot? It could help to see the previous status, if we've changed it (without saving). It's also in-line with e.g. the timestamp layout.

Another thing to consider are the icons. Currently the timestamp icon is added with CSS generated content. So it's natural that 43587.2.diff does the same for the status- and comments icons.

@afercia mentioned here:

"some screen readers already read out CSS generated content and more will do in the next future."

So should the icons be added via aria-hidden span tags here, instead of adding them via CSS generated content?

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

@nfmohit
6 years ago

Removes the comment status 'Edit' toggle and contains slight visual improvements and corrections

#10 @nfmohit
6 years ago

  • Resolution set to invalid
  • Status changed from new to closed

Thank you so much for checking the patch out @birgire !

@afercia suggested here (#2) that we should display the radio buttons by default and not use the extra "Edit" link to display it, for accessibility reasons.

I'm sorry I completely misunderstood that part. I initially intended to make the comment status radio buttons mimic the post status selection as we can see in the posts and pages, but after @afercia's comment, I went with the radio buttons instead of the selection. Didn't realize we were referring to the "Edit" toggle link. My bad. I have removed that in this patch 43587.3.diff.

On that screenshot I also added an extra left margin on the holders of the form inputs. What do you think? (I opened the timestamp edit, just to better show the alignment there)

I really like the idea of pushing the form inputs a little to the right aligning them with their section headers, but I haven't added it in the patch yet as I think we require more opinions on this. I believe if we are going to implement this, it'll also apply to the similar sections like post status and visibility selection in posts and pages, as we are trying to keep the layout consistent between comments and post types. If everyone is okay with it, I'll update my patch with that applied. Opinions?

Do we need the bold status text, shown in the screenshot? It could help to see the previous status, if we've changed it (without saving). It's also in-line with e.g. the timestamp layout.

I really prefer keeping the bold text showing the current comment status, it is static now though and doesn't change when a different status is selected (until the comment is saved, of course).

So should the icons be added via aria-hidden span tags here, instead of adding them via CSS generated content?

I'd also prefer adding the icons via aria-hidden span tags, but I also need more opinions here as CSS generated content is being used consistently in other sections like post status, post visibility and timestamp in post types.

Personally, I think if we are going to add the left margins to the form holders and change to using aria-hidden span tags for the icons, we'd like to do that globally in posts and other sections as well. Opinions?

Thank you so much!

#11 @birgire
6 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

@nfmohit I reopen the ticket, looks like you accidentally closed it as invalid ;-)

#12 @nfmohit
6 years ago

@birgire Whoops! I did? Yeah looks like I did. Sorry about that, it was a mistake. Thank you so much for reopening the ticket.

#13 @birgire
6 years ago

  • Owner set to nfmohit
  • Status changed from reopened to assigned

#14 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

#15 @pento
5 years ago

  • Keywords needs-design added

Patch needs design review.

#16 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

#17 follow-up: @melchoyce
5 years ago

Can someone add a screenshot of the latest patch?

@birgire
5 years ago

#18 in reply to: ↑ 17 ; follow-up: @birgire
5 years ago

Replying to melchoyce:

Can someone add a screenshot of the latest patch?

I refreshed the 43587.3.diff patch with 43587-4.diff.

Added the following screenshots:

#19 in reply to: ↑ 18 ; follow-up: @nfmohit
5 years ago

Replying to birgire:

Replying to melchoyce:

Can someone add a screenshot of the latest patch?

I refreshed the 43587.3.diff patch with 43587-4.diff.

Added the following screenshots:

Thank you for the refresh @birgire and apologies about the delay in response.

#20 in reply to: ↑ 19 @birgire
5 years ago

Replying to nfmohit:

Thank you for the refresh @birgire and apologies about the delay in response.

No problem and thanks for good points in comment:10.

I like the indentation on the comment's status choices and the comment's date fields.

To advance this ticket though, one might consider creating another ticket for the unified indentation?

#21 @afercia
5 years ago

Re: the "at" sign, it would be best to remove it and just use "at" in plain text. See https://core.trac.wordpress.org/ticket/44267#comment:10 :)

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


5 years ago

#23 follow-up: @melchoyce
5 years ago

The indentation is a little easier to scan, but it matches the Classic Editor, so let's move forward with the latest patch.

Maybe we could add indentation and swap out @ for "at" both here and in the Classic Editor in a new ticket.

#24 in reply to: ↑ 23 @birgire
5 years ago

Replying to melchoyce:

The indentation is a little easier to scan, but it matches the Classic Editor, so let's move forward with the latest patch.

Maybe we could add indentation and swap out @ for "at" both here and in the Classic Editor in a new ticket.

yes, that's sounds like a good plan to create separate tickets for the @ replacement and unified indentation.

I will look into that.

In before-and-after.jpg we can see the before and after according to edit-mode-from-43587-4.diff.jpg

Version 0, edited 5 years ago by birgire (next)

#25 @desrosj
5 years ago

  • Milestone changed from 5.2 to 5.3

The 5.2 beta deadline is just about here. Going to punt this one.

#26 @karmatosed
5 years ago

  • Keywords needs-design-feedback added; ui-feedback needs-design removed

This has design so removing that keyword and adding in design feedback one.

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


5 years ago

#28 @karmatosed
5 years ago

  • Keywords needs-design-feedback removed

This has design feedback from @melchoyce and I agree with her on the latest patch, for now, I am going to remove the feedback label.

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


5 years ago

#30 @birgire
5 years ago

Spinoff tickets:

  • #47832 for the @ replacement within the Post Publish Metabox.
  • #47893 for the @ replacement within the Comment Status Metabox.
Last edited 5 years ago by birgire (previous) (diff)

#31 @desrosj
5 years ago

  • Keywords needs-refresh added

Looks like edit-mode-from-43587-4.diff.jpg is not applying cleanly to the current trunk. Can someone please refresh?

@birgire
5 years ago

#32 @birgire
5 years ago

  • Keywords needs-refresh removed

@desrosj thanks for checking, patch refreshed in 43587-5.diff

#33 @desrosj
5 years ago

  • Milestone changed from 5.3 to 5.4

Unfortunately, with 5.3 beta 1 happening momentarily, this one needs to be punted.

#34 @davidbaumwald
4 years ago

  • Keywords needs-refresh added

The latest patch now fails against trunk. This one needs a quick refresh if it's to make it in to 5.4.

@birgire
4 years ago

#35 @birgire
4 years ago

  • Keywords needs-refresh removed

@davidbaumwald thanks, 43587-6.diff is a refresh.

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


4 years ago

#37 @davidbaumwald
4 years ago

  • Keywords commit added

Latest patch looks good. Marking for commit.

#38 @SergeyBiryukov
4 years ago

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

In 47252:

Comments: Improve the appearance of the Status box on Edit Comment screen.

This makes the box more consistent with the Publish meta box in classic editor.

Props birgire, nfmohit, melchoyce, afercia.
Fixes #43587.

Note: See TracTickets for help on using tickets.