Make WordPress Core

Opened 8 years ago

Closed 4 years ago

#37384 closed defect (bug) (fixed)

Text alignment in options-discussion.php

Reported by: ankit-k-gupta's profile Ankit K Gupta Owned by: whyisjake's profile whyisjake
Milestone: 5.5 Priority: low
Severity: minor Version: 4.5.3
Component: Administration Keywords: 2nd-opinion needs-testing has-patch
Focuses: ui, css, administration Cc:

Description

Labels are not aligned same as other labels.Having different margin due to <p> tag.
Attached patch file. I am beginner in CSS, may be this style need improvement.

Attachments (16)

BeforeFix.png (96.3 KB) - added by Ankit K Gupta 8 years ago.
Screenshot Before Fix
AfterFix.png (104.4 KB) - added by Ankit K Gupta 8 years ago.
Screenshot After Fix
37384.patch (3.5 KB) - added by Ankit K Gupta 8 years ago.
added patch
options-original-alignment.png (85.0 KB) - added by FolioVision 8 years ago.
Sample: how alignment in options currently looks
options-better-simpler-alignment.png (92.0 KB) - added by FolioVision 8 years ago.
How alignment looks after simplification, including wrapping TD content in P
options-discussion-alignment.html (11.1 KB) - added by FolioVision 8 years ago.
Sample simpler, cleaner options WP Admin code
37384.diff (650 bytes) - added by FolioVision 8 years ago.
before patch.png (26.6 KB) - added by Ankit K Gupta 8 years ago.
Screenshot without margin
after patch.png (24.5 KB) - added by Ankit K Gupta 8 years ago.
Screenshot After margin
37384-2.diff (663 bytes) - added by FolioVision 8 years ago.
table-spacing-fixed.png (81.8 KB) - added by FolioVision 8 years ago.
Targeting only select fields, that are inside label tags
Screenshot-WP-core.png (125.7 KB) - added by Ankit K Gupta 4 years ago.
wp-admin
Screen Shot 2020-02-13 at 11.47.40 AM.png (47.3 KB) - added by valentinbora 4 years ago.
This is still an issue on 5.4-beta1
37384-before-patch.png (122.5 KB) - added by samful 4 years ago.
37384-after patch.png (120.8 KB) - added by samful 4 years ago.
37384-after-patch-media.png (106.4 KB) - added by samful 4 years ago.

Download all attachments as: .zip

Change History (33)

@Ankit K Gupta
8 years ago

Screenshot Before Fix

@Ankit K Gupta
8 years ago

Screenshot After Fix

@Ankit K Gupta
8 years ago

added patch

#1 @Ankit K Gupta
8 years ago

  • Keywords has-patch 2nd-opinion added

#2 @SergeyBiryukov
8 years ago

  • Component changed from General to Administration

#3 @karmatosed
8 years ago

I think it would be great to get the alignment sorted, but I do wonder if this could be a CSS solution and keep the p tags for consistency. Have you explored that @Ankit K Gupta?

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


8 years ago

#5 @FolioVision
8 years ago

@Ankit, great that you opened up this ticket. The poor alignment in WordPress preferences drive me batty.

In some ways, it would be easier to remove the p tags as you recommend. Here's what our discovery found: sometimes things in right column are wrapped in p tags, but sometimes not (in Options -> Discussion the top ones). P tags as well as Label tags have their own margins, which add up. Plus, TH and TD don't have the same paddings, as well as TDs and P tags don't have the same line height. If you count this together, it's a mess.

Sample: how alignment in options currently looks

In line with @karmatosed's recommendation, we keep the p tag, I did some work on making preferences line up. In this case, every element is wrapped in P. For the moment it doesn't work with default WordPress admin but does work starting with a clean slate.

How alignment looks after simplification, including wrapping TD content in P

I'm attaching an HTML example of clean WordPress preferences with each the content in each td wrapped in P.

https://core.trac.wordpress.org/attachment/ticket/37384/options-discussion-alignment.html

.form-table cells include all different elements and tags. Right now many of them don't even share the same margins or paddings (like TH and TD). Every try to unify the tags one by one resulted in more work.

So we looked at what kind of scenarios happen inside table cells. In the end we found out, that the alignment improved by slightly changing the top margin on the Label tag. Then we added one extra declaration if a Label is placed inside a P tag and all of the cells aligned nicely. One more element changes regular paragraph text line heights on a regular basis - small select fields. If they are in first line of the text, it's breaking the alignment. Negative margin was added.

The big decision now is whether we should include P tags to wrap content in each TD or whether it would be better to only include P tags where absolutely required (no wrapping element). After this decision it will be hard slogging to clean up the WP Admin CSS to make the TD content align in all possible cases. There will need to be a lot of small changes (hopefully more removing properties than adding them).

Last edited 8 years ago by FolioVision (previous) (diff)

@FolioVision
8 years ago

Sample: how alignment in options currently looks

@FolioVision
8 years ago

How alignment looks after simplification, including wrapping TD content in P

@FolioVision
8 years ago

Sample simpler, cleaner options WP Admin code

@FolioVision
8 years ago

#6 @FolioVision
8 years ago

  • Keywords needs-testing added

After a second though I believe there is no need to start wrapping everything in P tags. Sometimes even regular paragraph text contains form field, that will further change line heights and alignment. So instead of putting everything in P, I propose a small change in forms.css. It aligns Label tag no matter if is wrapped in P or not, plus adds a fix for small select fields changing spacing when placed in P.

Diff included above

#7 @Ankit K Gupta
8 years ago

@karmatosed yep, Keeping p make sense to me. Nice suggestion. Thanks :)
@FolioVision Thanks for updating the patch and efforts, It looks fine on options-discussion.php page but it's creating little margin issue on options-media.php.
Please refer attached screenshots.
When we are using

margin-top: -4px;

on input.small-text class, it looks little miss-aligned

Last edited 8 years ago by Ankit K Gupta (previous) (diff)

@Ankit K Gupta
8 years ago

Screenshot without margin

@Ankit K Gupta
8 years ago

Screenshot After margin

#8 @Ankit K Gupta
8 years ago

  • Keywords needs-testing removed

@FolioVision
8 years ago

#9 @FolioVision
8 years ago

@Ankit K Gupta thanks for the testing, then we need to specify which scenario we want to tackle with that negative margin. I've updated my patch to work only if the input is inside a label tag.

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


8 years ago

@FolioVision
8 years ago

Targeting only select fields, that are inside label tags

#11 @Ankit K Gupta
7 years ago

  • Keywords needs-refresh added; has-patch removed

#12 follow-up: @desrosj
4 years ago

  • Keywords needs-screenshots added
  • Milestone set to Awaiting Review

with all of the admin styling adjustments in 5.3 and 5.3.1, it's likely that some of these issues may have been fixed.

Is anyone able to take some updated screenshots to demonstrate that these issues are still occurring?

#13 in reply to: ↑ 12 @Ankit K Gupta
4 years ago

  • Keywords needs-screenshots removed

Replying to desrosj:

with all of the admin styling adjustments in 5.3 and 5.3.1, it's likely that some of these issues may have been fixed.

Is anyone able to take some updated screenshots to demonstrate that these issues are still occurring?

Hello @desrosj

It looks little better but not fully aligned.
Please check attached screenshot.

Thanks!

@Ankit K Gupta
4 years ago

wp-admin

@valentinbora
4 years ago

This is still an issue on 5.4-beta1

#14 @valentinbora
4 years ago

  • Milestone changed from Awaiting Review to 5.5
  • Priority changed from normal to low
  • Severity changed from normal to minor

#15 @samful
4 years ago

  • Focuses css added
  • Keywords needs-testing has-patch added; needs-refresh removed

what was the reason @FolioVision 's https://core.trac.wordpress.org/attachment/ticket/37384/37384-2.diff wasn't used as the solution here? It seems to solve the issue for me. Did you try it @ankit-k-gupta ?

Screenshots below

#16 @whyisjake
4 years ago

  • Owner set to whyisjake
  • Status changed from new to accepted

Confirmed, still an issue in 5.5.

#17 @whyisjake
4 years ago

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

In 48419:

Administration: Better align labels on the discussion options page.

The labels for Comment Moderation, and Comment Blocklist are now aligned properly with the adjacent text.

Fixes #37384.
Props ankit-k-gupta, karmatosed, FolioVision, desrosj, valentinbora, samful, whyisjake.

Note: See TracTickets for help on using tickets.