Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#43586 closed enhancement (fixed)

Minor UI adjustments to the Author box in the Edit Comment screen

Reported by: birgire's profile birgire Owned by: pento's profile pento
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-screenshots has-patch commit
Focuses: ui, accessibility, administration Cc:

Description

Here are few suggestions for the Author box in the Edit Comment screen:

  • Add an underline for the Author title (like in meta-box headers).
  • Remove the colons as they are not (usually?) used before inputs in wp-admin.
  • Adjust the padding/margin, similar to other meta-boxes.

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

Attachments (19)

author-before.jpg (24.7 KB) - added by birgire 6 years ago.
author-after.jpg (24.0 KB) - added by birgire 6 years ago.
43586_1.diff (446 bytes) - added by abdullahramzan 6 years ago.
43586_2.diff (604 bytes) - added by abdullahramzan 6 years ago.
43586_3.diff (609 bytes) - added by abdullahramzan 6 years ago.
Balance the top bottom padding.
43586.diff (1.7 KB) - added by rhetorical 6 years ago.
43586.4.diff (1.7 KB) - added by rhetorical 6 years ago.
43586.5.diff (1.9 KB) - added by rhetorical 6 years ago.
43586.6.diff (2.9 KB) - added by rhetorical 6 years ago.
author-box-with-43586.6.diff.jpg (19.6 KB) - added by birgire 6 years ago.
author-box-in-mobile-viewport-with-43586.6.diff.jpg (21.6 KB) - added by birgire 6 years ago.
author-box-10px-padding.jpg (19.7 KB) - added by birgire 6 years ago.
43586.7.diff (3.3 KB) - added by rhetorical 6 years ago.
author-box-in-mobile-viewport-with-43586.7.diff.jpg (26.4 KB) - added by rhetorical 6 years ago.
43586.7-review-desktop.png (48.6 KB) - added by BODA1982 5 years ago.
Patch 43586.7 review on desktop
43586.7-review-mobile.png (46.9 KB) - added by BODA1982 5 years ago.
Patch 43586.7 review on mobile viewport
43586.8.diff (3.5 KB) - added by BODA1982 5 years ago.
43568.8-diff.png (85.1 KB) - added by BODA1982 5 years ago.
43586.2.diff (604 bytes) - added by afercia 5 years ago.

Download all attachments as: .zip

Change History (48)

@birgire
6 years ago

@birgire
6 years ago

#1 @birgire
6 years ago

  • Keywords has-screenshots added

The author-before.jpg screenshot shows the current layout.

The author-after.jpg shows a mockup example.

#2 @melchoyce
6 years ago

  • Keywords needs-patch good-first-bug added

This looks way better. Adding a divider and adjusting the spacing makes the form fields much more grounded in the container.

#3 @birgire
6 years ago

@melchoyce thanks for the feedback, I'm also creating a similar ticket for the Status box ;-)

#4 @birgire
6 years ago

Related #43587

#5 @abdullahramzan
6 years ago

  • Keywords has-patch added; needs-patch removed

Added following two patches:

  • 43586_1.diff for horizontal line.
  • 43586_2.diff for style.

@abdullahramzan
6 years ago

Balance the top bottom padding.

#6 @abdullahramzan
6 years ago

Added 43586_3.diff to balance the top bottom padding according to the design.
Consider 43586_3.diff and discard the 43586_1.diff

#7 @birgire
6 years ago

Thanks for the patches @abdullahramzan

Removing the <br /> looks good, but I'm not sure we should add a new <hr> tag for the underline. It seems to be more common to use e.g.:

border-bottom: 1px solid #eee;

as we can see for the meta-boxes.

It's also more convenient to have a single patch, to make it easier for others to test.

@rhetorical
6 years ago

@rhetorical
6 years ago

#8 @rhetorical
6 years ago

@birgire, sorry I did not realize that I there were tickets already uploaded. I have uploaded it with the correct diff numbering no please ignore my first one.

In regards to the margins for the the "Author" information, I've kept the left margin inline with the form fields. Please let me know if there are any questions.

#9 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.0

@rhetorical
6 years ago

#10 @rhetorical
6 years ago

better left padding fix on new 43586.5.diff

#11 @birgire
6 years ago

Thanks for the patch @rhetorical

I tested 43586.5.diff.

In regards to the margins for the the "Author" information, I've kept the left margin inline with the form fields.

I like it.

I think it would also be nicer if the amount of white space (around the form, below the border line) would be the same at top/bottom/left/right. What do you think?

But note that the patch also modifies all the meta-boxes on the post edit screen too. So that need to be avoided.

@rhetorical
6 years ago

#12 @rhetorical
6 years ago

Thanks for the review @birgire.

I agree with the changes on the white space around the form. I've made the adjustments there.

I've also updated the css so that I don't modify the meta-boxes css.

Let me know what you think or if you think anything else needs to be changed.

Thanks!

#13 @birgire
6 years ago

@rhetorical thanks for the update.

I noticed that the h2 header in meta-boxes have padding: 8px 12px but the content of (some) meta-boxes have padding: 10px.

I therefore wonder if the surrounding padding should also be 10px here, like shown in author-box-10px-padding.jpg, compared to the current layout from the 43586.6.diff patch: author-box-with-43586.6.diff.jpg

The mobile viewport needs adjustments (see author-box-in-mobile-viewport-with-43586.6.diff.jpg

#14 @rhetorical
6 years ago

@birgire thanks for the comments.

I believe you're right about the padding it should be 10px, I measured the padding on the form and 10 would align it perfectly left with the label.

In regards to the overall padding, textfields have a width of 98 percent, in general they would never meet up with the right padding. Should I make a special exception for this form only just so they match up with the 10px margin?

I'll also take a look at the mobile viewport, forgot about that one.

@rhetorical
6 years ago

#15 @rhetorical
6 years ago

I updated the padding to the 10px and adjusted the mobile viewport. I tried to match that up closely to the padding on the "quick-editor" comment form.

#16 in reply to: ↑ description @iticiti
6 years ago

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

#17 @pento
5 years ago

  • Milestone changed from 5.0 to 5.1

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


5 years ago

#19 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#20 @BODA1982
5 years ago

Tested attachment:43586.7.diff against trunk (5.1-alpha-20181129.2350335033) in browsers listed below on Mac. Working as advertised.

  • Chrome (70.0.3538.110)
  • Firefox (63.0.3)
  • Safari (11.1, not latest)

Looks good. Review shots below.

@BODA1982
5 years ago

Patch 43586.7 review on desktop

@BODA1982
5 years ago

Patch 43586.7 review on mobile viewport

#21 @birgire
5 years ago

Thanks for the testing @BODA1982 and the screenshots.

I noticed in 43586.7-review-mobile.png that the white space right under the URL input looks somewhat reduced. Was it the same in all the browsers you tested? I would vote for increasing this white space under the URL input to have it more symmetric.

@BODA1982
5 years ago

#22 @BODA1982
5 years ago

@birgire - attachment:43586.8.diff addresses the bottom white space. Matches up left/right white space in mobile viewport as well.

Edit/Note: I'm matching the bottom white space with the white space between the bottom border of "Author" and "Name". If you prefer something else, I'm happy to update it.

Version 2, edited 5 years ago by BODA1982 (previous) (next) (diff)

@BODA1982
5 years ago

#23 @pento
5 years ago

  • Owner changed from SergeyBiryukov to pento
  • Status changed from reviewing to accepted

#24 @pento
5 years ago

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

In 44466:

Comments: Improve the Author box in the Edit Comment screen.

  • Add a border below the heading, to match meta boxes.
  • Remove the colons from the input labels.
  • Tweak the padding and margins.

Props birgire, abdullahramzan, rhetorical, BODA1982, pento.
Fixes #43586.

#25 @afercia
5 years ago

  • Focuses accessibility added
  • Keywords needs-patch added; ui-feedback good-first-bug has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this ticket because I've just realised [44466] removed a fieldset legend and changed it to a heading, thus introducing an accessibility regression. Noticed while investigating on #33361.

The fieldset and legend elements were added in [32796] / #31326 with the specific goal to improve these form elements accessibility. It's always recommended to investigate a bit the code history to understand the reasons behind existing code.

The "Author" box has now a fieldset without a legend. This doesn't help so much assistive technologies users because the form elements are logically grouped within the fieldset but there's nothing to communicate what the group is about.

Regarding why a correct usage of fieldset and legend elements is important, I'd recommend this post by Léonie Watson on the gov.uk blog: https://accessibility.blog.gov.uk/2016/07/22/using-the-fieldset-and-legend-elements/

Also, the new heading is inside the fieldset and uses a span element, not sure why.

See also this related comment: https://core.trac.wordpress.org/ticket/33361#comment:12

I'd prefer to not enter into the debate if these are real meta boxes. I'd prefer to not use unnecessary headings too: headings should be used to identify content main sections, not for visual purposes. However, this change is in trunk and WordPress 5.1 is in Beta 2 phase so I'd suggest the most simple fix: use a visually hidden legend as already done in the "Status" box. It's a bit redundant, but it properly gives a name to the group of form elements.

To clarify visually: here's the Author fieldsed in 5.0.3 (left) and in trunk 5.1 (right), unstyled to make the underlying HTML semantics visible:

http://cldup.com/EOZRN4xrKA.png

@afercia
5 years ago

#26 @afercia
5 years ago

  • Keywords has-patch added; needs-patch removed

43586.2.diff does the following:

  • moves the h2 out of the fieldset
  • adds a visually hidden legend element
  • uses an existing string "Comment Author" already used in src/wp-includes/comment.php, if that works. If it doesn't I'm afraid it should be changed to "Author".

#27 @afercia
5 years ago

  • Priority changed from low to normal

#28 @pento
5 years ago

  • Keywords commit added

Nice catch, thanks @afercia. 43586.2.diff looks good to commit.

#29 @afercia
5 years ago

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

In 44712:

Comments: Restore a removed fieldset legend after [44466].

In the Edit Comment page:

  • moves the "Author" h2 heading out of the form fieldset
  • removes an unnecessary <span> element
  • adds a visually hidden legend element to the fieldset
  • uses an existing string "Comment Author"

Fixes #43586.

Note: See TracTickets for help on using tickets.