[wp-trac] [WordPress Trac] #25858: Integrate MP6 into core

[WordPress Trac]

#25858: Integrate MP6 into core
----------------------------+------------------
 Reporter:  dd32            |       Owner:
     Type:  task (blessed)  |      Status:  new
 Priority:  normal          |   Milestone:  3.8
Component:  General         |     Version:
 Severity:  normal          |  Resolution:
 Keywords:                  |
----------------------------+------------------

Comment (by dd32):

 Quick patch reviews. Not all of this is a blocker to commit, just
 mentioning what I can see, I understand most of this is not "production
 ready" and needs cleaning up still, but some of these things (particularly
 CSS) is going to be hard to do after it's commited as it often becomes
 lost and forgotten (and harder to spot without a diff).

 > Attachment 25858.colors.2.diff​ added
 * Skipping review of the grunt sections, I don't have a particularly good
 idea of what we can do there, seems like that's best left in another
 commit, but is kind of reququired by the picker.
 * .icon-dashicon & the dashicon() mixin is un-used, unreferenced styles in
 core and doesn't appear to be used by the patch
 * `src/wp-admin/includes/ajax-actions.php` needs cleaning up, error_log
 removal, nonce protection, cap checks to see if the current user can edit
 a user, questions on if we should even support changing another users
 colour scheme
 * Colour schemes are unavailable when running in `src`, need to lock all
 colour schemes to default for `src`.
 * docblock for `wp_admin_css_color()` needs updating with correct variable
 name, can probably convert that array( ..) to a compact() instead (would
 require renaming `$icon_colors`
 * `.picker-dropdown` in `admin_color_scheme_picker()` HTML appears to be
 indented an extra tab, should also use the `selected()` helper, and
 `esc_url()` for URL escaping (instead of `esc_attr()`

 > Attachment 25858.responsive.diff​ added
 * In addition to the above list from tollmanz:
 * Would be nice to standardise on CSS layout, This one doesn't indent
 rules within media queries
 * `.rtl` will need to be removed
 * Jetpack and Akismet rules need removing, if those rules are needed, then
 something more generic which applies to all plugins should be investigated
 * That's 2,000 lines of CSS that I can't review, a few things feel like
 things that we removed in late 3.7 as they weren't needed.. not sure.
 though..
 * It'd be nice if we could clean up this CSS before dumping it into core,
 just to prevent it being difficult in the future to determine where/when
 the code was added.. But it's such a huge changeset that ultimately I
 don't think it's goig to matter, it's not dispersed amongst other code..
 * JS looks fine, it needs the eyes of a JS master though for performance
 and probably JSHint alterations

 > Attachment 25858.5.diff​ added
 Commit it already! :)

 > Attachment 25858.widgets.diff​ added
 * Vastly different CSS structure than is used elsewhere in core, and in
 other MP6 patches (super-indenting each subsequent "nested" rule)
 * Would've been nice if this CSS was modified in-place rather than being
 copy-pasted into a new block, makes it rather hard to review what actually
 changed, for example, I can see that `.widget_title h4` is mostly the
 same, but it moved for no reason, etc.
 * Colours need moving to colour stylesheets, no need to `!important` it
 here
 * There's so much new CSS here it seems, that splitting it into it's own
 file is fast approaching, it's 1,000 lines of altered CSS atm.

 > Attachment 25858.4.diff​ added
 Needs commiting.

--
Ticket URL: <http://core.trac.wordpress.org/ticket/25858#comment:30>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software