Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 8 years ago

#26669 closed enhancement (fixed)

wp-admin.css should be broken up into modules

Reported by: jorbin's profile jorbin Owned by: helen's profile helen
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: ui Cc:

Description (last modified by jorbin)

This is a follow up #22862.

There helen stated:

Nah, let's address autoprefixing and concat separately and in between cycles / the very start of a new one - too much churn to do it right now without killing everybody's patches.

This ticket adds concatenation. And right now we are at the start of a new cycle, so I figured why not.

Attachments (13)

modules.diff (485.3 KB) - added by jorbin 11 years ago.
modules.2.diff (485.6 KB) - added by jorbin 11 years ago.
modules_without_css.diff (5.1 KB) - added by jorbin 11 years ago.
26669.diff (521.1 KB) - added by jorbin 11 years ago.
26669.2.diff (520.7 KB) - added by jorbin 11 years ago.
26669.3.diff (9.8 KB) - added by helen 11 years ago.
26669.4.diff (3.7 MB) - added by helen 11 years ago.
26669.5.diff (3.8 MB) - added by helen 11 years ago.
26669.6.diff (3.8 MB) - added by helen 11 years ago.
26669-gruntfile.diff (1.7 KB) - added by helen 11 years ago.
26669.7.diff (3.8 MB) - added by helen 11 years ago.
wp-admin-css-dir.zip (74.5 KB) - added by helen 11 years ago.
26669-gruntfile.2.diff (2.3 KB) - added by nacin 11 years ago.

Change History (37)

#1 @jorbin
11 years ago

  • Keywords has-patch added
  • Type changed from defect (bug) to enhancement

The above patch doesn't change any of our css, all it does is move it into smaller more manageable chunks and adds the grunt pieces along with the special src magic inside wp-includes/script-loader.php

#2 @jorbin
11 years ago

  • Description modified (diff)

@jorbin
11 years ago

#3 @SergeyBiryukov
11 years ago

  • Component changed from General to Administration

#4 follow-up: @nacin
11 years ago

The one concern I have about this is it makes it much more difficult to submit a CSS patch via core.svn. An alternative approach could be to have all of these files in both core.svn and develop.svn, then also include a single wp-admin.min.css file in the built version. SCRIPT_DEBUG would load these individual files in both build and src.

Seems like if we have separate files, then each should be commented, and the TOC should be removed.

TinyMCE can just be removed, that is covered by wp-includes/css/editor.css.

It will be easier to examine any future patches if wp-admin.css just got comments added throughout, as in: /* this would start wp-admin-buttons.css, forms.css, etc. */, rather than actually moving code around. Or if the CSS files are admitted entirely for now, in lieu of the grunt/script-loader/etc changes. The TOC can be patched with /* this would be split into buttons.css, */ or something, that way we can discuss organization.

#5 in reply to: ↑ 4 @jorbin
11 years ago

Replying to nacin:

The one concern I have about this is it makes it much more difficult to submit a CSS patch via core.svn. An alternative approach could be to have all of these files in both core.svn and develop.svn, then also include a single wp-admin.min.css file in the built version. SCRIPT_DEBUG would load these individual files in both build and src.

Seems like if we have separate files, then each should be commented, and the TOC should be removed.

That makes a lot of sense. I've switched it so SCRIPT_DEBUG always loads all the files. I've kept TOC since I think having a reference of what is where makes sense. Though it's entirely possible that it could easily get stale and may make more sense to remove it.

@jorbin
11 years ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


11 years ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


11 years ago

#9 @helen
11 years ago

I think we should probably make sure wp-login.php-specific styles are separate and usable separately or at least investigate what to do there: #12506.

@jorbin
11 years ago

@jorbin
11 years ago

#10 @jorbin
11 years ago

  • Focuses ui added

The above patch divides wp-admin.css up and also fixes #12506.

Note, that I tried to include a blank wp-login.css file in src/wp-admin/css , but it doesn't seem to actually get added when I apply my only patch. That should also be a part of it.

This ticket was mentioned in IRC in #wordpress-dev by jorbin_work. View the logs.


11 years ago

This ticket was mentioned in IRC in #wordpress-dev by jorbin_work. View the logs.


11 years ago

@helen
11 years ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


11 years ago

#14 @helen
11 years ago

26669.3.diff is a quick split proposal using comments in wp-admin.css, a lot of it based on jorbin's work. Some thoughts:

  • The number of times you will see some markers repeated (common, forms, etc.) is a pretty clear indicator of the mess we're in.
  • I'm leaving plugins-related CSS as part of list tables as well as common (the plugin details view).
  • Themes is separate from appearance, due to the amount of CSS and assuming further changes due to continued work to align the manage and install screens.
  • Not sure if users-tools-settings should be separate, or just put into common for now. it's a fairly small amount of CSS.
  • Press This has two blobs, one currently indicated as part of settings (see above), the other as part of common. Not sure what to do with it.

#15 @nacin
11 years ago

In 27181:

Rename the old media.css file, used for the pre-3.5 media library. Clean up script-loader CSS registrations.

see #26669.

@helen
11 years ago

@helen
11 years ago

@helen
11 years ago

@helen
11 years ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


11 years ago

#17 @helen
11 years ago

  • Owner set to helen
  • Resolution set to fixed
  • Status changed from new to closed

In [27195/trunk/Gruntfile.js]:

Once upon a time not long ago,
The admin CSS was merged in #18314.
After a couple years as it stood,
The mess it had become just was no good.
One day we realized Grunt is pretty cool,
And said "we should use this as our build tool!"
Now we can maintain separate files with ease,
Using @import and cssmin meets all our needs.
Welcome to the future of the WordPress stylesheets,
And thanks to Slick Rick for the beats.

props jorbin for the initial patch.
fixes #26669.

Last edited 11 years ago by nacin (previous) (diff)

#18 @helen
11 years ago

  • Milestone changed from Awaiting Review to 3.9

A few things here:

  1. Do not attempt to load the changeset in Trac or link to it directly. At last count, the number of altered lines was 213,216, due to using svn cp to create each of the new CSS files with history.
  2. wp-admin.css uses @import for each of the modules/components/whatever we call them, cssjanus creates RTL versions of each, and wp-admin-rtl.css is created, calling the generated RTL files.
  3. The hit of multiple HTTP requests only applies with SCRIPT_DEBUG on - clean-css (via cssmin) inlines @imported files in the minified versions.
  4. This approach works out well for us because it does not involve script loader changes, it utilizes existing tooling, core.svn and develop.svn can continue to both be used for patches, and web inspectors handle CSS @imports already - no need to worry about whether or not source maps are supported yet.

Please open new tickets for any issues discovered, as it may not be clear whether it is a consequence of this split or the preceding colors.css merge in #18380.

#19 follow-up: @johnjamesjacoby
11 years ago

Using @import and cssmin meets all our needs

@import can be problematic for plugins that include their own admin color schemes because wp-content might not always be relative to wp-admin.

Helen: we've talked in the past about just copying dependent styles directly into the plugin's color scheme(s), but that comes with added overhead of needing to merge core styling changes between versions. Because it changes frequently (and drastically every few years) I was hoping for an approach that enqueues the structural bits, and separates out the individual color schemes.

Any comment or recommendation on how best to proceed, and does this point warrant its own ticket?

#20 in reply to: ↑ 19 ; follow-up: @jorbin
11 years ago

Replying to johnjamesjacoby:

Using @import and cssmin meets all our needs

@import can be problematic for plugins that include their own admin color schemes because wp-content might not always be relative to wp-admin.

@import is just being used by wp-admin/css/admin.css at this point and unless plugins are hacking core, I'm confused as to how this will affect them. For people running without SCRIPT_DEBUG set to true, this creates zero change for them.

Version 0, edited 11 years ago by jorbin (next)

#21 in reply to: ↑ 20 ; follow-up: @johnjamesjacoby
11 years ago

Replying to jorbin:

@import is just being used by wp-admin/css/wp-admin.css at this point and unless plugins are hacking core, I'm confused as to how this will affect them. For people running without SCRIPT_DEBUG set to true, this creates zero change for them.

The commit message (though clever, and quoting Children's Story is always appreciated) wasn't super clear (to me) about what was switched to using @import. After reading Helen's summary, it wasn't much more clear what the impact would be for plugins that include their own color schemes -- something that has been historically difficult (see: almost impossible without including your own image assets, and without pulling in colors-fresh for 3.8.)

Showing compassion for plugin developers that want to do good and ensure limited/no breakage (especially when 200k line changesets come in) goes a long way towards keeping up-stream contributions flowing in. I'm hoping the next time someone asks a similar question, it's met with a more positive and helpful response.

Having done some cursory testing with bbPress's green schemes, I'll sign off that all appears a-okay. I'll create a new ticket if I see any oddities. Nice work, Helen.

#22 in reply to: ↑ 21 @helen
11 years ago

Replying to johnjamesjacoby:

Having done some cursory testing with bbPress's green schemes, I'll sign off that all appears a-okay. I'll create a new ticket if I see any oddities. Nice work, Helen.

Thanks, glad to hear that things are looking okay. I readily admit that (a) I got too excited about my commit message and just committing the blasted thing and failed to make a plain English summary until later, and (b) because this particular changeset only affects core development, I didn't think to allay plugin/theme dev concerns with something so large that you can't even view it. But trust me: this has zero impact on plugin and theme development. :)

All the CSS changes move existing CSS into separated files and break up the media queries so the cascade isn't broken. The Gruntfile.js changes can be found in 26669-gruntfile.2.diff. Finally, a few lines were removed from $_old_files, because some newly created CSS files have the same name as files that existed before the 3.3 merge, so we don't want to copy the new files over and then have the install process just stupidly delete them again (it's happened before). Happy to diff that file alone for the curious. Here's what changed:

M       Gruntfile.js
A  +    src/wp-admin/css/about.css
A  +    src/wp-admin/css/admin-menu.css
A  +    src/wp-admin/css/common.css
A  +    src/wp-admin/css/dashboard.css
A  +    src/wp-admin/css/edit.css
A  +    src/wp-admin/css/forms.css
A  +    src/wp-admin/css/l10n.css
A  +    src/wp-admin/css/list-tables.css
A  +    src/wp-admin/css/login.css
A  +    src/wp-admin/css/media.css
A  +    src/wp-admin/css/nav-menus.css
A  +    src/wp-admin/css/press-this.css
A  +    src/wp-admin/css/revisions.css
A  +    src/wp-admin/css/themes.css
A  +    src/wp-admin/css/widgets.css
M       src/wp-admin/css/wp-admin.css
M       src/wp-admin/includes/update-core.php

#23 @nacin
11 years ago

In 27228:

Make sure color schemes are registered when WP_Styles is initialized early.

fixes #27175. see #26669.

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


8 years ago

Note: See TracTickets for help on using tickets.