Fix "moby6"

[azaozz]

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 and deactivate custom jQuery events.
• Global custom jQuery events are better attached to document, not documentElement.
• 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 uses if ( typeof ... !== 'undefined' ). Also there is no stickymenu JS global in core, the name is stickyMenu.
• 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.

[helen]

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 :(

[tollmanz]

@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.

[tollmanz]

The patch described in the previous message

[azaozz]

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 like touch-helper (touchHelper in JS), or maybe wp-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.

[azaozz]

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').

[azaozz]

In 26402:

Clean up the sticky menu and responsive tweaks JS:

• More descriptive names.
• Share cached jQuery elements.
• Fix re-enabling of touch events on the admin menu.
• Fix disabling/enabling of UI Sortable.

Props tollmanz, props sanchothefat, see #26086, fixes #26088.

[azaozz]

In 26421:

Remove cloning of the search box with JS and move it to the bottom of the screen with CSS instead, see #26086

[azaozz]

In 26516:

Make the responsive menu usable with a mouse, fix non-folded and :focus styles, toggle the submenus on touchend/click. Fixes #26086.

[atimmer]

[26516] introduced a jshint error, ​26086.jshint.diff fixes it.