#26086 closed defect (bug) (fixed)
Fix "moby6"
Reported by: | azaozz | Owned by: | azaozz |
---|---|---|---|
Milestone: | 3.8 | Priority: | highest omg bbq |
Severity: | minor | Version: | 3.8 |
Component: | Administration | Keywords: | |
Focuses: | ui | Cc: |
Description
The MP6 merge introduced a component (moby6) that needs quite a lot of fixing. The JS part of it (currently in common.js) is mostly "hacks", good for a testing plugin but not for core. Most of the functionality there can be done better with CSS.
- Most names are non descriptive ("moby6" sounds like a plugin's prefix?). Some names are quite poor for use in core, like
activate
anddeactivate
custom jQuery events. - Global custom jQuery events are better attached to
document
, notdocumentElement
. - Disabling jQuery UI sortable is done with
$(selector).sortable('disable')
not by changing the handle elements classes. Also there is some JS making it (mostly) possible to use UI Sortable on touch devices. If that is not needed anywhere, better to remove it. - Syntax like
window.stickymenu && window.stickymenu.enable();
is not particularly readable, core usesif ( typeof ... !== 'undefined' )
. Also there is nostickymenu
JS global in core, the name isstickyMenu
. - Cloning an element from JS, appending it to a different place in the DOM and then hiding the original is a really hacky way to move the search box on some screens. If it really really has to be done from JS, the original can me moved, no need of cloning.
- The
removeHamburgerButton
function doesn't do anything.
Attachments (2)
Change History (11)
#2
@
11 years ago
@azaozz - I've started on some cleanup of this. After this comment, I will attach a patch. I was waiting to upload this patch and start a ticket until I got a member of the MP6 team to review it to make sure I am not introducing regressions. While I can see a lot of JS issues with this code, it is hard to change as it is difficult to grok what this code is doing. That said, the forthcoming patch addresses the following:
- Removes
moby6
language - Adds better selector caching
- Fixes the
stickymenu/stickyMenu
issues - Adds underscore.js as a dependency for common.js in order to use the
debounce
method - Removes the
removeHamburgerButton
button, which was previously missed - Deprecates
matchMedia
as it appears unnecessary in this case
This gets the ball rolling, but obviously needs to address more of your concerns. I would like to be able to have someone articulate what this code is supposed to do, then improve it to do these things better. Additionally, we need better documentation of these functions to better describe its purpose. I think that is the toughest part of improving this code.
#3
@
11 years ago
Yep, it's starting to look a lot better. Some more thoughts:
responsive
still sounds too "global". I'm not that good at naming too but maybe something liketouch-helper
(touchHelper
in JS), or maybewp-touch-menus
as it deals primarily with touch devices and the menu. Maybe @helen would have a better idea :)- Consider merging "moby6" and stickyMenu. They both deal with similar things and share cached elements.
- Not sure we need to include underscore.js on absolutely all admin pages just for a simple timing function. On one side Underscore is small and has many useful utility functions, on the other side most of these functions are also in jQuery which is already available everywhere. The "fire once at end of window resize" (BTW why fire this on window.scroll, doesn't do anything) is pretty easy to do with fixed timeout and callback, something like:
// `i` is just for testing, paste in the browser console and try it var timeout, i = 0; function fireOnce() { clearTimeout( timeout ); timeout = setTimeout( function() { i++; console.log('run = '+i); }, 200 ); } jQuery(window).resize( fireOnce );
- When adding custom events to jQuery, best is to add them on
$(document)
and name them descriptively. Namespacing is also a very good idea (would say "required") but only when attaching to events, not when adding them. So$(document.documentElement).trigger('activate.responsive');
would be better as$(document).trigger('wp-touch-menus-activate');
or whatever name we end up choosing. Also, are these custom events needed elsewhere? If not, perhaps they can be simple callbacks scoped in the wrapper function. - Perhaps the scope can be managed better by declaring all cached jQuery elements outside the mixin. That would also share them with
stickyMenu
if we decide not to merge these two.
#4
@
11 years ago
- Priority changed from normal to highest omg bbq
- Severity changed from normal to minor
#5
@
11 years ago
Related: #26088.
Working on a patch that extends 26086.diff, scoping it better so cached jQuery elements are shared with stickyMenu, replacing the Underscore debounce() with a custom jQuery event fired at the end of window resize (as it is useful elsewhere) and using $(container).sortable('disable').
#8
@
11 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 26516:
#9
@
11 years ago
[26516] introduced a jshint error, 26086.jshint.diff fixes it.
Yeah, we need to un-codename this and continue pruning out as much as we can. tollmanz helped us cut out about half of the JS already (it previously used Backbone and jQuery Mobile). Also, the stickymenu/stickyMenu thing is my fault :(