Refactor: Create subcomponent "ui.menubarMenuItem"

[sgharms]

Per conversation #953 and email with @jzaefferer, I was feeling very frustrated with the very long procedural style of initializing the components in the menubar. In a spike to see if I could make the code's initialization more understandable, I tried out this refactor and it really feels like a much cleaner initialization process.

To wit, https://github.com/sgharms/jquery-ui/pull/new/jquery:menubar...refactor#L1R164 shows a much nicer initialization process ( https://github.com/sgharms/jquery-ui/pull/new/jquery:menubar...refactor#L1R221 ) since we have a class that manages all the setup and event application logic.

It does, however, create a widget called ui.menubarMenuItem. I feel that the gain in having a class which can be pointed at within the context of ui.menubar is well worth constructing the code in this fashion.

However I do not want to create this widget as public, I'd like to keep it as a "private inner widget" to menubar. I'm not sure of the best way to do that. Conceivably I could delete it from the $ object after assigning it to a private variable inside menubar? I'm open to better ways of doing it.

In any case, I believe the code is now much more debuggable and much easier to understand. This setup will also provide a much better interface for mocking and stubbing in the expanded unit test suite requested by http://wiki.jqueryui.com/w/page/38666403/Menubar.

This PR is less "I want this merged in" and more of a "is this way better" but having looked at this code this morning for a few hours, I despair at ever going back to what's in jquery-ui/menubar.

[scottgonzalez]

To be honest, I find it hard to give a reasonable review of this because the code that is being refactored is so far from our normal style. But at first glance, I still feel like this is moving further away from our normal style.

[scottgonzalez]

This is the type of thing that makes me not even want to review the full code.

[sgharms]

Could you be a bit more specific?

1. Is it the call to menubar per se?
2. Is it the way the injection into menu works i.e. the code introduced in ba8f147 ?
3. Is it the wrapping of this action set in a semantically meaningful method name?

[scottgonzalez]

Is it the call to menubar per se?

Yes.

Is it the way the injection into menu works i.e. the code introduced in ba8f147 ?

There was no new code introduced there, it's a revert of the deletion of the entire plugin. If you're trying to point out something specific, that commit is not useful.

Is it the wrapping of this action set in a semantically meaningful method name?

Yes.

[sgharms]

Amended SHA should be: 8a0df90.

That commit from two years ago to the day had effectively the same logic. Can you explain your position further on the items which you responded to with "Yes?" Specifically what is wrong? Is it preferable to use a keyword like self versus menubar? Is it preferable for JQuery UI components to use a single long procedural style (as was the case when I first encountered it in November)?

[scottgonzalez]

I'm not sure what injection you're referring to in that commit. How are the accessibility changes related to splitting out menubaritem? I'm not talking about variable names, I'm talking about the level of indirection (pulling options form the parent widget); the creation of new widgets that we don't event want to expose; the creation of a function to access a single, unprocessed, option; the extremely verbose naming of a function; etc.

There's a difference between procedural code and long functions. I'm still not sure what specific problems you're trying to fix. You said this "shows a much nicer initialization process". The initialization process doesn't seem to have changed much, other than being split into a ton of functions. You say the benefit comes from having "a class that manages all the setup and event application logic." That's what menubar is.

[sgharms]

Well, maybe we should step back and take a look at my motivations. I
fundamentally believe that code should be:

• Easy to read for someone unfamiliar with the code base
• Easy to extend

The former means that projects can move forward with new effort and that future
selves can easily regain the context of the code. The latter means that
features take time to implement proportional to their complexity.

So, when I think about a Menubar I expect it to be composed of Menus. This
would lead to an initalization routine of the form

_create: function() {
  this._initMe();
  this._initTheThingsIManage({ class: ComposingClass });
}

Uncoincidentally, the code in the menubar widget in this PR looks something
like that. Given $.Widget being a means for encapsulating behavior (other
languages might call it being a factory for classes), I used $.Widget,
not because I wanted to create a widget, but because I wanted to have an
instance which encapsulated this cognitive payload of work.

In constrast, when I first visited menubar code back in Nov 2012, the _create
method didn't look like the above, where there was a top level "outline" of
what was contained within( 0d30bda ). When I attempted to make my first bugfix
(what became 9f53e0a), I could not readily grasp the nature of what was going
on or where to inspect the erroneous close behavior. A few lines should
suffice to express my point:

  _create: function() {
    var that = this;
    this.menuItems = this.element.children( this.options.items );
    this.items = this.menuItems.children( "button, a" );

    this.menuItems
      .addClass( "ui-menubar-item" )
      .attr( "role", "presentation" );
    // let only the first item receive focus
    this.items.slice(1).attr( "tabIndex", -1 );

    this.element
      .addClass( "ui-menubar ui-widget-header ui-helper-clearfix" )
      .attr( "role", "menubar" );
    this._focusable( this.items );
    this._hoverable( this.items );
    this.items.siblings( this.options.menuElement )
      .menu({
        position: {
          within: this.options.position.within
        },
        select: function( event, ui ) {
          ui.item.parents( "ul.ui-menu:last" ).hide();
          that._close();
          // TODO what is this targetting? there's probably a better way to access it
          $(event.target).prev().focus();
          that._trigger( "select", event, ui );
        },
        menus: that.options.menuElement
      })
      .hide()

Maybe it's because I'm dimmer than the rest of you all, but having the code
document its intent (perhaps in function calls with (arguably) overlong names),
is very helpful for me when reading this code. My cognitive focus faded
about the time this.items.siblings was invoked.

So these were my goals with this change. If you have any constructive feedback
about whether this PR moves closer to my stated goals I'd be glad to hear it.
Or, perhaps you can explain the superiority of the code style that I cited
above. It may have virtues that I'm not readily seeing but I consider it a
smell when one method does so very much.

[scottgonzalez]

As I said, even the old code doesn't follow our normal style. Note that this is still outside of master and I have not reviewed it yet.

Have you taken a look at other plugins like tabs or dialog? The _create() function shouldn't be 1 or 2 lines. Creation IS initialization, so having it call another function which does the initialization is pointless.

[sgharms]

I concur, from the interpreter's point of view there's no difference between the two.

But humans of limited intellect, such as myself, greatly appreciate having things work at high levels of abstraction unto lower. I will take a look at dialog as you recommend.

[sgharms]

@jzaefferer has provided some background on how widgets are constructed:

• $.Widget is not actually meant to be used for a class factory
• Widget design should be more self-contained and monolithic

Closing and will adjust elsewhere. No harm in trying something new. 🐤