WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 8 months ago

#43586 closed enhancement (fixed)

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

Reported by: birgire Owned by: 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 18 months ago.
author-after.jpg (24.0 KB) - added by birgire 18 months ago.
43586_1.diff (446 bytes) - added by abdullahramzan 18 months ago.
43586_2.diff (604 bytes) - added by abdullahramzan 18 months ago.
43586_3.diff (609 bytes) - added by abdullahramzan 18 months ago.
Balance the top bottom padding.
43586.diff (1.7 KB) - added by rhetorical 18 months ago.
43586.4.diff (1.7 KB) - added by rhetorical 18 months ago.
43586.5.diff (1.9 KB) - added by rhetorical 18 months ago.
43586.6.diff (2.9 KB) - added by rhetorical 18 months ago.
author-box-with-43586.6.diff.jpg (19.6 KB) - added by birgire 18 months ago.
author-box-in-mobile-viewport-with-43586.6.diff.jpg (21.6 KB) - added by birgire 18 months ago.
author-box-10px-padding.jpg (19.7 KB) - added by birgire 18 months ago.
43586.7.diff (3.3 KB) - added by rhetorical 18 months ago.
author-box-in-mobile-viewport-with-43586.7.diff.jpg (26.4 KB) - added by rhetorical 18 months ago.
43586.7-review-desktop.png (48.6 KB) - added by BODA1982 10 months ago.
Patch 43586.7 review on desktop
43586.7-review-mobile.png (46.9 KB) - added by BODA1982 10 months ago.
Patch 43586.7 review on mobile viewport
43586.8.diff (3.5 KB) - added by BODA1982 10 months ago.
43568.8-diff.png (85.1 KB) - added by BODA1982 10 months ago.
43586.2.diff (604 bytes) - added by afercia 8 months ago.

Download all attachments as: .zip

Change History (48)

#1 @birgire
18 months ago

  • Keywords has-screenshots added

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

The author-after.jpg shows a mockup example.

#2 @melchoyce
18 months 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
18 months ago

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

#5 @abdullahramzan
18 months ago

  • Keywords has-patch added; needs-patch removed

Added following two patches:

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

@abdullahramzan
18 months ago

Balance the top bottom padding.

#6 @abdullahramzan
18 months 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
18 months 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
18 months ago

@rhetorical
18 months ago

#8 @rhetorical
18 months 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
18 months ago

  • Milestone changed from Awaiting Review to 5.0

@rhetorical
18 months ago

#10 @rhetorical
18 months ago

better left padding fix on new 43586.5.diff

#11 @birgire
18 months 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
18 months ago

#12 @rhetorical
18 months 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
18 months 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
18 months 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
18 months ago

#15 @rhetorical
18 months 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
16 months ago

Last edited 16 months ago by iticiti (previous) (diff)

#17 @pento
12 months ago

  • Milestone changed from 5.0 to 5.1

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


10 months ago

#19 @SergeyBiryukov
10 months ago

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

#20 @BODA1982
10 months 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
10 months ago

Patch 43586.7 review on desktop

@BODA1982
10 months ago

Patch 43586.7 review on mobile viewport

#21 @birgire
10 months 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
10 months ago

#22 @BODA1982
10 months 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.

Last edited 10 months ago by BODA1982 (previous) (diff)

#23 @pento
9 months ago

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

#24 @pento
9 months 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
8 months 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
8 months ago

#26 @afercia
8 months 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
8 months ago

  • Priority changed from low to normal

#28 @pento
8 months ago

  • Keywords commit added

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

#29 @afercia
8 months 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.