Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#52148 closed enhancement (fixed)

CSS Optimization: Use shorthand 'border' value in wp-admin/css/admin-menu.css

Reported by: ankitmaru's profile ankitmaru Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.7
Component: Administration Keywords: has-patch commit
Focuses: ui, css, coding-standards Cc:

Description

Properties can be replaced with 'border' shorthand.

Attachments (13)

admin-menu.css.patch (546 bytes) - added by ankitmaru 4 years ago.
class-wp-comments-list-table.php_52148.patch (557 bytes) - added by ankitmaru 4 years ago.
common.css_52148.patch (880 bytes) - added by ankitmaru 4 years ago.
customize-controls.css_52148.patch (770 bytes) - added by ankitmaru 4 years ago.
edit.css_52148.patch (447 bytes) - added by ankitmaru 4 years ago.
install.css_52148.patch (364 bytes) - added by ankitmaru 4 years ago.
nav-menus.css_52148.patch (404 bytes) - added by ankitmaru 4 years ago.
css_52148.patch (3.3 KB) - added by ankitmaru 4 years ago.
css_up_52148.patch (3.4 KB) - added by ankitmaru 4 years ago.
css_final_52148.patch (3.4 KB) - added by ankitmaru 4 years ago.
css_updated_52148.patch (3.1 KB) - added by ankitmaru 4 years ago.
css_updated_01_02_2021_52148.patch (4.4 KB) - added by ankitmaru 4 years ago.
52148.diff (3.8 KB) - added by hellofromTonya 4 years ago.
Refreshed to (a) change 0px to 0 and (b) remove class-wp-comments-list-table.php changes

Download all attachments as: .zip

Change History (40)

#1 @ankitmaru
4 years ago

  • Keywords has-patch added

#2 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.7

#3 @SergeyBiryukov
4 years ago

  • Summary changed from CSS Optimization to CSS Optimization: Use shorthand 'border' value in wp-admin/css/admin-menu.css

#4 @SergeyBiryukov
4 years ago

  • Keywords commit added

#5 follow-up: @audrasjb
4 years ago

Hi, thanks for the ticket and patch,

I think it would be nice to quickly check other occurence of this.
For example, in wp-admin/css/common.css:

.contextual-help-tabs li {
	margin-bottom: 0;
	list-style-type: none;
	border-style: solid;
	border-width: 0 0 0 2px;
	border-color: transparent;
}
Last edited 4 years ago by audrasjb (previous) (diff)

#6 @audrasjb
4 years ago

Hum… looks like it's legitimate in this case, though… because there is a different border for each side…

#7 @sabernhardt
4 years ago

Besides jquery-ui-dialog.css, I found one additional instance that can combine all three properties into one line:
wp-includes/css/editor.css

#wp_editimgbtn,
#wp_delimgbtn,
#wp_editgallery,
#wp_delgallery {
	background-color: #eee;
	margin: 2px;
	padding: 2px;
	border: 1px solid #999;
	border-radius: 3px;
}

Border styles like .contextual-help-tabs li (comment:5) could combine to two lines, if desired.

#8 in reply to: ↑ 5 @ankitmaru
4 years ago

I have added few more patches as well. Checked whole css/ folder for the same improvements.
Replying to audrasjb:

Hi, thanks for the ticket and patch,

I think it would be nice to quickly check other occurence of this.
For example, in wp-admin/css/common.css:

.contextual-help-tabs li {
	margin-bottom: 0;
	list-style-type: none;
	border-style: solid;
	border-width: 0 0 0 2px;
	border-color: transparent;
}

#9 @SergeyBiryukov
4 years ago

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

#10 @audrasjb
4 years ago

  • Keywords commit removed

@ankitmaru it would be easier for committers to merge all those patch in one unique patch, if possible :)

#11 @ankitmaru
4 years ago

@audrasjb Sorry for that, Here I am attaching one unique patch.

@ankitmaru
4 years ago

#12 @audrasjb
4 years ago

Thanks for the quick update @ankitmaru you rock!

One small change on my side:

  • the /* equal height column trick */ comment should be kept (in common.css file). As this is a small CSS "hack", it's better to keep it documented ;-)

#13 @ankitmaru
4 years ago

@audrasjb Thanks

#14 @ankitmaru
4 years ago

@audrasjb Sorry for too many patches, Now it looks good to go. Thanks

#15 @audrasjb
4 years ago

  • Keywords commit added

@ankitmaru there’s not need to be sorry, thank you for helping this patch to move forward!

The patch looks good to me. Marking this for commit.

#16 @ankitmaru
4 years ago

@audrasjb Thank you so much.

#17 @audrasjb
4 years ago

  • Keywords commit removed

I'm really sorry @ankitmaru but I have few more requests:

  • in common.css replace padding: 10px 26px 99999px 26px; with padding: 10px 26px 99999px; / and padding: 16px 16px 99999px 16px; with padding: 16px 16px 99999px;
  • In customize-controls.css replace padding: 0px 0px 7px 0px; with padding: 0 0 7px;
  • In install.css replace padding: 0px 0px 7px 0px; with padding: 0 0 7px;
  • In nav-menus.css replace margin: 0px 0px 5px 0px; with margin: 0 0 5px;

See MDN Docs for further informations:
https://developer.mozilla.org/en-US/docs/Web/CSS/padding#syntax

The padding [it also applies for margin] property may be specified using one, two, three, or four values.

  • When one value is specified, it applies the same padding to all four sides.
  • When two values are specified, the first padding applies to the top and bottom, the second to the left and right.
  • When three values are specified, the first padding applies to the top, the second to the right and left, the third to the bottom.
  • When four values are specified, the paddings apply to the top, right, bottom, and left in that order (clockwise).
Last edited 4 years ago by audrasjb (previous) (diff)

#18 @ankitmaru
4 years ago

@audrasjb updated the patch.

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


4 years ago

#20 @mukesh27
4 years ago

  • Keywords needs-refresh added

I review the patches for the ticket and found the mention issues.

customize-controls.css_52148.patch

This padding: 0px 0px 7px 0px; should replace with padding: 0px 0px 7px;

class-wp-comments-list-table.php_52148.patch

As per our docs standard return should be at last not at first in the documentation.

install.css_52148.patch

This padding: 0px 0px 7px 0px; should replace with padding: 0px 0px 7px;

nav-menus.css_52148.patch

This margin: 0px 0px 5px 0px; should replace with margin: 0px 0px 5px;

#21 follow-up: @sabernhardt
4 years ago

@ankitmaru Thanks for combining the patches and updating it in css_updated_52148.patch.

Please make two more changes:

  1. For the second change in customize-controls.css, remove the units with a zero value:
    margin: 0 0 0 10px;
    

(see coding standards about zero value)

  1. Combine the 3 border properties in wp-includes/css/editor.css (comment:7)

If you don't have time to update the patch today, I could do that.

#22 in reply to: ↑ 21 @ankitmaru
4 years ago

I have fixed the point no. 2, Can you please share more info about point no. 1?
Replying to sabernhardt:

@ankitmaru Thanks for combining the patches and updating it in css_updated_52148.patch.

Please make two more changes:

  1. For the second change in customize-controls.css, remove the units with a zero value:
    margin: 0 0 0 10px;
    

(see coding standards about zero value)

  1. Combine the 3 border properties in wp-includes/css/editor.css (comment:7)

If you don't have time to update the patch today, I could do that.

#23 @sabernhardt
4 years ago

The styles for #customize-theme-controls .add-new-menu-item do not need the px units for the three zero values, because the result is the same no matter what the unit is.

#customize-theme-controls .add-new-widget,
#customize-theme-controls .add-new-menu-item {
	cursor: pointer;
	float: right;
	margin: 0 0 0 10px;
	transition: all 0.2s;
	-webkit-user-select: none;
	-ms-user-select: none;
	user-select: none;
	outline: none;
}

(You already did similarly with the body.cheatin h1 padding earlier in that file.)

padding: 0 0 7px;

Also, I don't think the class-wp-comments-list-table.php change belongs on this ticket. After putting the @global int $post_id first among the global variables, you could upload a patch for that file to ticket:51800.

@hellofromTonya
4 years ago

Refreshed to (a) change 0px to 0 and (b) remove class-wp-comments-list-table.php changes

#24 @hellofromTonya
4 years ago

  • Keywords commit added; needs-refresh removed

Today is 5.7 Beta 1. I refreshed the patch per @sabernhardt last comment.

Marking this as a commit candidate.

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


4 years ago

#26 @SergeyBiryukov
4 years ago

Thanks for the patches! This looks good to me, but I would skip the changes in wp-admin/css/common.css.

That "trick" looks more readable to me as is, and since we're not combining margin properties there, I don't see a point in combining padding.

#27 @jorbin
4 years ago

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

In 50162:

Administration: use shorthand css properties to improve readability

Consolidating border, padding, and margin instances where the shorthand can be used to improve readability.

Props ankitmaru, audrasjb, sabernhardt, mukesh27, hellofromTonya.
Fixes #52148.

Note: See TracTickets for help on using tickets.