#25858 closed task (blessed) (fixed)
Integrate MP6 into core
Reported by: | dd32 | Owned by: | |
---|---|---|---|
Milestone: | 3.8 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | |
Focuses: | Cc: |
Description
As discussed during the 3.8 planning sessions, 3.8 is bringing the MP6 Plugin into core for a UI refresh and flattening.
Lets make it happen!
Attachments (31)
Change History (119)
@
11 years ago
mp6.2.diff + Removal of -fresh & -classic laying MP6 down as the "fresh" to make way for MP6's colour schemes
#2
@
11 years ago
mp6.2.diff was a refinement of the first patch, primarily some spacing and a few merge conflicts I missed (included both old and new css rules).
mp6.3.diff as the description implies, is mp6.2.diff and removing the colors-* stylesheets (since they're no good with MP6), brings core to a single consistent colour scheme until the colour scheme picker gets merged.
#10
@
11 years ago
Wanted to note that Open Sans is actually loading from Google for now, due to issues being spotted with extended character sets that need further investigation and resolution.
Also noting that MP6 the plugin will continue to function just fine, it just loads some unnecessary things for trunk, and until other components are merged in, still improves certain experiences, like responsive.
@
11 years ago
Updated patch with a comment and ref to the original ticket so that future generations will know what this does.
#19
follow-ups:
↓ 20
↓ 21
↓ 23
@
11 years ago
25858.6.diff is a first pass at the merge of the sticky menu component into core. In the merge process, I have updated a few things:
- I added a debounce function to the resize and scroll events. It is important that this is throttled
- Everything was cleaned up to pass the jshint rules
- All repeated selectors were cached
A few questions about this:
- Given the integration into core via add JS to an existing JS file and CSS to an existing CSS file, I removed the PHP related to this component that only served to load the assets. One piece of that PHP is that it did not load this JS/CSS on mobile other than iPad. I could not figure out why this was a requirement. Is this needed? Obviously, where possible, especially with a project like WordPress, browser sniffing is not desirable, so it would be nice to remove this altogether, but I do not have an idea as to the implications of it.
- I stole Underscore.js's debounce function. I did not want to include Underscore.js as it would make Underscore.js a dependency for all of WP admin. Any thoughts use of this function?
#20
in reply to:
↑ 19
@
11 years ago
Replying to tollmanz:
- I stole Underscore.js's debounce function. I did not want to include Underscore.js as it would make Underscore.js a dependency for all of WP admin. Any thoughts use of this function?
Minimized Underscore is 14k. We already are including the 91k jQuery throughout the admin, so it's not a huge addition in my eyes. I know there are plenty of places underscore makes sense to use and this could encourage us to use it more.
#21
in reply to:
↑ 19
@
11 years ago
Replying to tollmanz:
One piece of that PHP is that it did not load this JS/CSS on mobile other than iPad. I could not figure out why this was a requirement. Is this needed?
That code has been ported from the stand-alone plugin to prevent position: fixed
issues on mobile devices and can be removed. MP6's responsive component disables the sticky-menu on smaller screens in moby6.js
.
#22
@
11 years ago
25858.colors.diff merges in the colors component.
- Add a folder to src/wp-admin/css with the Sass source files for color schemes.
- Add a new task to the Gruntfile:
colors
, which runs the Sass processing & CSS minification, and add this task tobuild
. This does require Sass (and therefore Ruby) on the machine running grunt, specifically v 3.3.0 as we want to use the sourcemap option. - Each processed color scheme generates a colors.css, colors.css.map, and colors.min.css in the build directory, along with keeping the colors.scss files.
This also changes how color schemes are handled in core
- There is a new property to each color scheme object,
icon_colors
. This is an array of colors used by svg-painter to match any SVG icons to the color scheme. - The color scheme picker has a new style & now auto-saves.
A few notes
- New function
set_color_scheme_json
creates the json objectmp6_color_scheme
used by svg-painter, and both should be renamed to something not MP6. - Color schemes won't work if you're running out of the src directory. Maybe we need to put some error handling there like (now) exists for RTL, if you're using a color scheme. And probably disable the color scheme picker.
#23
in reply to:
↑ 19
@
11 years ago
Replying to tollmanz:
25858.6.diff is a first pass at the merge of the sticky menu component into core.
The z-index
styles for pointers/thickbox in attachment:25858.6.diff need to be merged properly, those are initial stand-alone hot fixes.
#24
follow-up:
↓ 27
@
11 years ago
25858.colors.diff merges in the colors component.
Add a new task to the Gruntfile: colors, which runs the Sass processing & CSS minification, and add this task to build. This does require Sass (and therefore Ruby) on the machine running grunt, specifically v 3.3.0 as we want to use the sourcemap option.
- It's a shame we can't use libsass instead of using ruby sass, but as pointed out elsewhere, libsass isn't feature compatible yet, and doesn't support sourcemaps
- @nacin can probably weigh in here a bit, but I'm wondering if perhaps we should commit the generated .css files from the scss files to /src/ ? I guess only having the base colour scheme defined is OK though, but, it's something that might be more useful in the future when other stylesheets are made with a pre-processor
More General:
- What files were
src/wp-admin/css/color-schemes/_admin.scss
&src/wp-admin/css/color-schemes/_mixins.scss
based off? I'd rather do asvn cp
on the file so that we can see only the variables have been replaced, and no rules were lost/added - I can see that'll be a messy diff due to the mixin separation, but the declarations shouldn't change - Is there a reason why the colors.scss files appear to have duplicated content (ie. double imports of colors-fresh.css, and the blue scheme particularly)?
- Probably best to add one colour scheme, and then
svn cp
the rest (once again, to see whats different between them) - The
.dropdown
and.expanded
styles are incredibly generic, can they be prefixed with something specific to the job? - (not specific to this patch) What's the job of
svg-painter.js
? It seems like it might only be used by the colour scheme picker? (Or does it actually do something on load too?) I ask since we're currently loading it on all pages
#26
@
11 years ago
Attachment 25858.6.diff added
Took a look at this, few things that we can change from MP6:
- We can remove the .rtl rule, that's no longer needed
- We don't need to go all-out-crazy on the z-indexing as far as I can see, this works for me: (collapsing code to limit the comment length)
.sticky-menu #adminmenuwrap { position: fixed; top: 32px; left: 0; z-index: 2; } .sticky-menu #wpwrap { z-index: 1; }
Avoiding crazy z-indexes is something we really need to strive against, otherwise we just add something starting at 9000 next time..
#27
in reply to:
↑ 24
@
11 years ago
Replying to dd32:
More General:
- What files were
src/wp-admin/css/color-schemes/_admin.scss
&src/wp-admin/css/color-schemes/_mixins.scss
based off? I'd rather do asvn cp
on the file so that we can see only the variables have been replaced, and no rules were lost/added - I can see that'll be a messy diff due to the mixin separation, but the declarations shouldn't change
Do you mean in the plugin? components/color-schemes/schemes/_admin.scss
& components/color-schemes/schemes/_mixins.scss
.
- Is there a reason why the colors.scss files appear to have duplicated content (ie. double imports of colors-fresh.css, and the blue scheme particularly)?
No, that was a mistake from my testing. Fixed in 25858.colors.2.diff.
- Probably best to add one colour scheme, and then
svn cp
the rest (once again, to see whats different between them)- The
.dropdown
and.expanded
styles are incredibly generic, can they be prefixed with something specific to the job?
Changed the classes to picker-dropdown
/picker-expanded
in 25858.colors.2.diff.
I also noticed that there are still moby6
classes in _admin.scss
, those should be removed once we know what's up with the responsive component.
#29
@
11 years ago
- Cc tollmanz@… added
25858.responsive.diff brings in the responsive component of MP6. The primary goal of this patch is to port this code to core, warts and all. We believe that this brings parity to the responsive component in MP6.
In this process, we decided to make a few changes:
- Removed jQuery mobile. This script was used to add swipe controls to open/close the sidebar menu. This feature was apparently buggy and due to the pending demise of jQuery mobile, it was removed.
- Removed use of Backbone.js. Adding Backbone.js to this script would add a dependency of Backbone.js for all of the admin. Additionally, it was used to add a menu item. Instead of doing that, it was added via the admin menu API. This also fixes a bad delay in the item showing in the menu.
While this patch ports the responsive component into core, there is much work to be done, including:
- CSS code styling. A ton of CSS was dumped into wp-admin.css, but was not cleaned up. There is a lot of inconsistency with it and should be fixed up.
- CSS organization. We should revisit if the CSS is in the proper place. It might make sense to reorganize it a bit now that this is core code.
- Remove moby6 references. No need for this anymore.
- jshinting. We should clean up JS to pass jshint tests.
- Throttle resize event. We need to make sure these event don't hose performance.
- Review use of matchMedia. I think that we could deprecate this and make it pure CSS.
- Menu icon alignment in Chrome is a bit off and needs tweaked.
- Sidebar quirkiness. On load in narrow view, it will initially pop out, then hide. There are lots of little quirks like this that need identification and fixing.
- Overall review of code. There are likely other issues that need addressing and this code warrants a thorough review.
Finally, I kept a log of changes we made to this patch. It gives the history of moving from the MP6 port to its current state. This history can be viewed here: https://gist.github.com/tollmanz/7442994/revisions. The following is a guide to these changes:
- Port of responsive component to core
- Removes jQuery mobile from patch
- Removes Backbone.js requirement from patch
- Styles the new menu icon
- Fixes a bug that hid the admin menu
Props to iammattthomas, helen, dd32, and tollmanz for work on this.
#30
follow-ups:
↓ 33
↓ 34
@
11 years ago
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 forsrc
. - 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
inadmin_color_scheme_picker()
HTML appears to be indented an extra tab, should also use theselected()
helper, andesc_url()
for URL escaping (instead ofesc_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.
#33
in reply to:
↑ 30
@
11 years ago
Some quick fixes in 25858.colors.3.diff
- .icon-dashicon & the dashicon() mixin is un-used, unreferenced styles in core and doesn't appear to be used by the patch
I think it might be worth having dashicon classes in core, but this isn't the place for that- this has been removed.
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
Agreed. Removed error_log, more cleanup to come. I think you've always been able to change another user's color scheme, but the autosave here might make that dangerous (unexpected).
- 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
)
Fixed the docblock name.
.picker-dropdown
inadmin_color_scheme_picker()
HTML appears to be indented an extra tab, should also use theselected()
helper, andesc_url()
for URL escaping (instead ofesc_attr()
)
The 'selected' code is just for a class, so selected()
isn't appropriate here. I've updated the escaping for the URL to use esc_url
.
#34
in reply to:
↑ 30
@
11 years ago
Updated the widgets patch:
- Vastly different CSS structure than is used elsewhere in core, and in other MP6 patches (super-indenting each subsequent "nested" rule)
I've cleaned this up and reinstated "normal" nesting of rules, no super-indenting.
- 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.
I looked into this a bit, but it seems like an attempt was made to more logically organize the CSS along with the changes that were made. While it makes reviewing this diff a bit more difficult, I think the organization makes sense.
- 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.
Going to leave these two for post-merge fixes if there are no objections.
#35
@
11 years ago
25858.responsive.2.diff addresses issues brought up by dd32:
- CSS layout is standardized. Comments have also been cleaned up
- Jetpack and Akismet code is removed
- rtl CSS is removed
- JS passes hinting other than one small issue that will likely be removed when parts of the code are reviewed
@
11 years ago
This adds to tollmanz's latest responsive patch, fixing the positioning of the adminbar Updates icon and removing the hover state on the hamburger button.
#36
@
11 years ago
Latest responsive patch removes user-scalable=no and the maximum zoom from the viewport declaration.
#37
@
11 years ago
Replying to dd32:
Attachment 25858.responsive.diff added
...
JS looks fine, it needs the eyes of a JS master though for performance and probably JSHint alterations
Looking through the JS in 25858.responsive.4.diff: it will need a lot of of work after the merge. Most of what it does can be done (better?) with @media from CSS.
#38
@
11 years ago
Looking through the JS in 25858.responsive.4.diff: it will need a lot of of work after the merge. Most of what it does can be done (better?) with @media from CSS.
I'm in 100% agreement with this. Check that, 150% agreement.
I tried to rewrite the JS as I prepped the patch; however, I was introducing new bugs left and right. In order to make the deadline and at least get us to a state consistent with the current state of MP6, we decided to port everything over as is (with the necessary changes mentioned above). My goal is to gradually rewrite this code over the next few weeks. I would love your keen eye on any patches to see if you think I'm heading in the right direction.
@
11 years ago
This is a minor change from the last widgets patch which fixes a bug that broke click-to-add widgets.
@
11 years ago
Available widget descriptions are now always visible. The .more-info ? and tooltip are gone.
@
11 years ago
One more widgets patch, this time with styles for the oft forgotten accessibility mode.
@
11 years ago
Customizer: remove gradients & add a border to distinguish customizer panel from site content
#51
@
11 years ago
- Resolution set to fixed
- Status changed from new to closed
I think that completes the merge. Going to call this fixed - new tickets for individual bugs, please.
#65
@
11 years ago
@iammattthomas in r26423 you used the 600px breakpoint to prevent horizontal scrolling when the responsive menu is open, but the menu is shown at 782px or less. See 25858.7.diff, unless that was intentional.
#78
@
11 years ago
I'm a bit worried the change from #eee
to #f3f3f3
could have more implications to the overall luminance balance. It's currently throwing off some shadows and some borders with the fainter look, and can make the white-boxes elements we use throughout the admin less visible — specially on old screens, that cannot handle subtleties in the highlights very well. Why can't we just change the About page background if that was the concern?
#79
@
11 years ago
It's not just the About page that felt heavy; it's also pages like Settings that don't use the white boxes. I haven't seen the problems with contrast or shadows that matveb mentioned but I think at this point we've reached the point of diminishing returns; there are tradeoffs either way.
#80
follow-up:
↓ 81
@
11 years ago
If we want to split the difference, we can use #f1f1f1 as a page background. I've used this a hundred times in the past and it seems to provide adequate contrast with a white container on top of it, especially since ours have darker borders and shadows. 25858.10.diff makes that change.
#81
in reply to:
↑ 80
@
11 years ago
- Cc asmussen@… added
I find the reasoning very convincing, and the change is certainly something I can live with. I'd like to echo Matias thoughts though, that especially with #f3f3f3 we lose some of the glorious contrast MP6 introduced. #f1f1f1 would make me significantly happier, though, and seems like the best of both worlds.
#84
@
11 years ago
25858.11.diff drops down the border-color and box-shadow a bit on container elements to match the new #f1f1f1 background-color.
mp6.diff is a merge of MP6 as of r800128.
The patch assumes:
A few extra points:
wp-admin.css
andcustomize-controls.css
, the diff is messy here as this was rewritten with SCSS, only the produced CSS is included here, we can split it out and add a SCSS/LESS processor here (another ticket please)There's still a bunch more spacing issues in the patch though, I'll upload a 2nd pass at it soon.