Make WordPress Core

Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#20570 closed enhancement (maybelater)

Tidying Admin CSS, bringing it up to CSS Code Standards (work in progress)

Reported by: georgestephanis's profile georgestephanis Owned by:
Milestone: Priority: normal
Severity: minor Version: 3.4
Component: Administration Keywords: work-in-progress
Focuses: ui Cc:

Description (last modified by SergeyBiryukov)

as per http://codex.wordpress.org/CSS_Coding_Standards, this is an attempt to bring the internal admin stylesheets up to CSS Coding Standards

Some of the changes being made are ...

  • Properly intenting top: left: right: bottom: attributes two tabs when following a position:absolute position:fixed or position:relative declaration
  • Alphabetizing css properties
  • Removing unnecessary browser prefixes
  • Placing browser prefixed css properties before the non-browser prefixed versions
  • Removing extraneous spaces
  • Applying a consistent ordering of browser-prefixed properties (based on length of prefix to get an aesthetically pleasing diagonal line, rather than jagged, inconsistent line endings -- yes, I know it's silly, but it's better than randomness, and alphabetical isn't as useful)
  • Applying a uniform method to attribute selectors and url references by wrapping double-quotes around the values (there was no prior standard and a motley assortment of usages)
  • Changing colors to 3-character shorthand ( 888888 becomes 888 )
  • Changing colors to lower case ( EEE becomes eee )

and probably a couple others that aren't coming to mind right now.

Attachments (2)

20570.draft.diff (79.5 KB) - added by georgestephanis 12 years ago.
First draft -- more revisions pending
hex-not-name.patch (3.2 KB) - added by bananastalktome 12 years ago.

Download all attachments as: .zip

Change History (37)

@georgestephanis
12 years ago

First draft -- more revisions pending

#1 @SergeyBiryukov
12 years ago

  • Description modified (diff)

#2 @ocean90
12 years ago

Are these standards now final?

#3 @georgestephanis
12 years ago

There's some bits and pieces that aren't specified in it -- I've got a poll running at http://stephanis.info/wp-css-coding-standards/ to try and get a consensus.

As far as whether it is final, I'm not certain. If not, I'd like to get them solidified one direction or another, so we don't have three ways of doing things.

If possible, I'd also like to include in the standards which properties we encourage browser-prefixing, as well as which ones we discourage.

border-radius and box-shadow have progressed to a point where neither really need the -moz- and -webkit- browser prefixing any longer, IMHO.

#4 @ocean90
12 years ago

Related: #19114

#5 @azaozz
12 years ago

Some of the so proposed "coding standards" seem pretty weird imho. Keep in mind that most people look at the CSS code through Firebug and friends, making the formatting somewhat redundant:

  • "Properly indenting top: left: right: bottom: attributes...". Why? It's not more readable.
  • "Alphabetizing css properties". What's the point? We don't alphabetize function names/placement in PHP and JS...
  • "Removing unnecessary browser prefixes". Sounds good as long as no currently user browser is left behind.
  • "Placing browser prefixed css properties before the non-browser prefixed versions". This is more of a requirement than code style. As far as I remember there was a case when this didn't work properly.
  • "Removing extraneous spaces". As long as that doesn't impact readability. Perhaps leaving two empty lines between sections.
  • "Applying a consistent ordering of browser-prefixed properties". Meaning -moz- should always go above -webkit-? Same as alphabetizing, don't see the point.

Also thinking there should be another one: "Each selector should be on a new line".

As far as I understand it "Coding Standards" means "to make the code readable in a consistent way". Alphabetizing usually goes against that. It seems better if the css properties are grouped by functionality rather than alphabetized. For example top: and left: should be right after position:, padding: should be next to margin:, etc.

#6 follow-up: @georgestephanis
12 years ago

I'm not particularly attached to alphabetizing -- if we want to ditch it, it's fine by me. I started down this rabbit hole looking to address the inconsistency in whether the value should be quoted in [type="number"] or not, and just got sucked in.

It's kinda like this => http://cl.ly/2j2g2f2Q0t191E1Z2a3n

#7 in reply to: ↑ 6 @azaozz
12 years ago

Replying to georgestephanis:

Ah, I see...

Yeah, agree that discussing coding standards on trac seems better than relaying on people editing the wiki directly. After all most people that will have to comply with the standards seem more likely to comment here :)

Right, we need to follow the specs for always quoting [type="..."] and url("..."). Also when using hex color values, they seem easier to read when lower case and shortened. Thinking we still need some of the -webkit- stuff for the mobile versions of Safari.

#8 @DrewAPicture
12 years ago

  • Cc xoodrew@… added

Also not really on board with alphabetizing. I agree with @azaozz that several of these are a bit weird and somewhat unnecessary.

As for using shorthand with property values, some are easy to navigate and others aren't. There seems to be some contention that shorthand is more difficult to read. I personally like it better.

Maybe should also add something about how to handle indenting @media queries and sub-selectors since they've been sneaking into core in the last couple of releases.

#9 @sabreuse
12 years ago

  • Cc sabreuse@… added

#10 follow-up: @bananastalktome
12 years ago

  • Cc bananastalktome@… added

Patch which changes css color names to their hex counterparts (let me know if I got any wrong, and even if this patch is not used at least fix .wp-dialog in wp-includes/css/editor.dev.css since an invalid color value is used)

#11 follow-ups: @helenyhou
12 years ago

I am also not a big fan of alphabetizing and a couple other points, but I know I've seen... disagreements from the past about these coding standards that never seem to have resolved. There is definitely a need to actually come to an agreement. Have already talked briefly to georgestephanis about this in #wordpress-ui - perhaps we could actually hold a meeting during the regularly scheduled time and at least start talking it through? And perhaps also grab the Automatticians/Theme Team who are also (to my knowledge) working on their coding standards?

Let's hold off on patches for now, though. They will quickly become stale and are not useful without a decision. This is the sort of work that really needs to happen pretty much directly after a stable version drops and I think we'll find that there is a general CSS cleanup needed, during which we can also look at including standards fixing. It'll muck up some of the revision history/blame, but so it goes.

Also, bananastalktome - note that your patch doesn't always use the shortened hex value. We should probably fix that value for .wp-dialog, though...

#12 in reply to: ↑ 10 ; follow-up: @SergeyBiryukov
12 years ago

Replying to bananastalktome:

at least fix .wp-dialog in wp-includes/css/editor.dev.css since an invalid color value is used)

I guess it's better to create a separate ticket for that.

#13 @SergeyBiryukov
12 years ago

+1 to lower case and shortened colors and always quoting [type="..."] and url("...").

−1 to additional tab for top: left: right: bottom: and alphabetizing properties.

#14 in reply to: ↑ 12 @bananastalktome
12 years ago

Replying to SergeyBiryukov:

Replying to bananastalktome:

at least fix .wp-dialog in wp-includes/css/editor.dev.css since an invalid color value is used)

I guess it's better to create a separate ticket for that.

created (#20574)

#15 @bananastalktome
12 years ago

In looking through the stylesheets, I noticed some discrepancies with the text-shadow property. In some instances, we have things like text-shadow: 0 -1px 0 #333; and other times we have text-shadow: #fff 0 1px 0; (color first vs color last). The w3c validator (http://jigsaw.w3.org/css-validator/validator) appears to normalize the css to using color first when I pasted in a few different snippets from the WordPress css, but I have seen sources online showing both ways as correct. Is there a preference on this (and is it worthy of another patch to normalize)?

#16 in reply to: ↑ 11 @bananastalktome
12 years ago

Replying to helenyhou:

Also, bananastalktome - note that your patch doesn't always use the shortened hex value. We should probably fix that value for .wp-dialog, though...

Good call, it was an oversight on my part. Is this worth me revisiting to fix, or should I just hold off until some consensus is reached on whether this should be a rule?

#17 follow-up: @georgestephanis
12 years ago

I think we'll wait until CSS is fairly stable for a couple weeks (probably right after the release of 3.4) to do a larger overhaul -- if it is to happen at all. In the mean time, our focus would be best spent on building a consensus with regard to what principles to use in the overhaul.

#18 in reply to: ↑ 17 ; follow-up: @azaozz
12 years ago

Replying to helenyhou:
Replying to georgestephanis:

I wouldn't call them "disagreements", rather ideas and personal preferences :)

Really like the idea to talk things over in #wordpress-ui, next meetup there is tomorrow (Tuesday) at 18:00 UTC, right?

Yes, most of the admin CSS is still in need of a general overhaul/cleanup, it's too much to try to do all at once, so I see it more like an ongoing process. Would be great if we have written CSS coding standards before attempting the next round, probably early in 3.5.

#19 in reply to: ↑ 18 @helenyhou
12 years ago

Replying to azaozz:

Yep, Tuesdays at 18:00 UTC is what it says, so let's go with that. Shall I post on Make UI about it? I have a couple other things I want to run by the group as well, and it might be worth putting a call out to more prospective contributors in the HTML/CSS area and also do a little discussion about testing RTL and colors and such so it doesn't get left for the very very end like usual.

#20 @azaozz
12 years ago

Sounds good. Yes, the UI group seems to be busiest near a new release, but perhaps can plan some tasks that would be better done early in a release cycle, like general css handling in core, RTL, etc.

Last edited 12 years ago by azaozz (previous) (diff)

#21 in reply to: ↑ 11 @koopersmith
12 years ago

Replying to helenyhou:

I am also not a big fan of alphabetizing and a couple other points, but I know I've seen... disagreements from the past about these coding standards that never seem to have resolved. There is definitely a need to actually come to an agreement.

Let's hold off on patches for now, though. They will quickly become stale and are not useful without a decision. This is the sort of work that really needs to happen pretty much directly after a stable version drops and I think we'll find that there is a general CSS cleanup needed, during which we can also look at including standards fixing.

I couldn't agree more. Let's start up the meetings again and try to come to a consensus on an updated CSS style guide early next cycle.

#23 @nacin
12 years ago

  • Type changed from defect (bug) to enhancement

#25 @helenyhou
11 years ago

#22097 was marked as a duplicate.

#27 @SergeyBiryukov
11 years ago

  • Component changed from General to Administration
  • Keywords ui-focus added

#28 @helen
11 years ago

Anybody interested in this should probably take a look at MP6 and get involved through there for now.

#29 @ocean90
10 years ago

  • Keywords close added

Not sure what we should do with this ticket, helen?

#30 follow-up: @helen
10 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

I don't know. I don't see any realistic path to this, as lovely as it sounds. Let's maybelater this, maybe over time as we do things like split files, introduce reusable components, and take advantage of tooling, we can naturally ease in some sanity.

#31 in reply to: ↑ 30 ; follow-up: @GaryJ
10 years ago

Replying to helen:

I don't know. I don't see any realistic path to this, as lovely as it sounds. Let's maybelater this, maybe over time as we do things like split files, introduce reusable components, and take advantage of tooling, we can naturally ease in some sanity.

Specifically regarding property ordering, CSSComb has been rewritten as JS, so it's available as a grunt task (and also available as a Sublime Text plugin, and also the csscomb.com website, once they've finished rebuilding it). I'd love to see this integrated, and any .csscomb.json file in core referenced from the CSS Coding Standards Handbook so that plugin and theme developers can grab a copy and use it in their own projects.

The CSS Coding Standards in the Handbook currently gives the property ordering:

  • Display
  • Positioning
  • Box model
  • Colors and Typography
  • Other

This isn't prescriptive enough to be useful.

The advantage of having a consistent order of properties is reduced maintenance (it works on source files, so properties could be ordered pre-commit too) and to gain more benefit when gzipping.

As an example, CSSComb can take:

.foo:before {
	content: 'foo';
	background: #424242;
	width: 18px;
	border-radius: 10px;
	font-size: 11px;
	line-height: 18px;
	color: #fff;
	height: 18px;
	content: 'foo';
	text-align: center;
	display: inline-block;
}

and order it to:

.foo:before {
	display: inline-block;
	width: 18px;
	height: 18px;
	font-size: 11px;
	line-height: 18px;
	color: #fff;
	border-radius: 10px;
	background: #424242;
	content: 'foo';
	text-align: center;
}

Properties are individually orderable, so if you want text-align up next to font-related properties, it can be.

The output can optionally contain groupings (might be nice for .scss files):

.foo:before {
	display: inline-block;

	width: 18px;
	height: 18px;

	font-size: 11px;
	line-height: 18px;

	color: #fff;
	border-radius: 10px;
	background: #424242;

	content: 'foo';
	text-align: center;
}

Any reason not to implement this tool to automate a consistent order of properties?

#32 in reply to: ↑ 31 ; follow-up: @helen
10 years ago

Replying to GaryJ:

CSSComb would be one of the tools I have in mind for exploring, yes. There's no reason not to, although I'd question if it's the right first step toward getting our CSS cleaned up.

#33 in reply to: ↑ 32 ; follow-up: @GaryJ
10 years ago

Replying to helen:

Replying to GaryJ:

CSSComb would be one of the tools I have in mind for exploring, yes.

That's good to know. I'll see if I can do a first-pass suggestion at a config file then. Beyond that, what's the best place (and with whom) to discuss the actual order of individual properties?

There's no reason not to, although I'd question if it's the right first step toward getting our CSS cleaned up.

Since this is only changing the order of existing properties, it doesn't really matter if it's done first, or done once some of it has been removed / simplified / moved to Sass. As such, I'd rather see something get started on it, and the order agreed / config decided, even if WP doesn't include the grunt task within existing build tasks until later.

#34 @helen
10 years ago

#28040 was marked as a duplicate.

#35 in reply to: ↑ 33 @GaryJ
10 years ago

Replying to GaryJ:

I'll see if I can do a first-pass suggestion at a config file then.

https://gist.github.com/GaryJones/c1fc8f0243406b9190c8

The rules should be self-explanatory, and match up to the CSS standards in the core handbook. The top two values may need configuring or dropping, so I've left them separated at the top.

I've left clear line spaces within the property ordering so you can see how they match up to the five sections in the handbook. Since the properties are all in a single array, they won't end up with clear line spaces in the final output. The first section is extra, and is for .scss. It has no effect on plain .css files, so it's fine to leave in.

I'd started with https://github.com/csscomb/csscomb.js/blob/master/config/csscomb.json and adjusted from there, so if there are any properties missing, let me know.

Note: See TracTickets for help on using tickets.