Tabs: Improve styling, adding line breaks on multiple chains

[jzaefferer]

Stricter adherence to style guide, not necessarily in improvement in readability though.

[jzaefferer]

From #1494, Scott wrote:

---

That's pretty random - both find and filter change context.

And .not() and .children() and lots of other methods. I thought this existing block was valid:

this.tabs.not( this.active ).attr( {
    "aria-selected": "false",
    "aria-expanded": "false",
    tabIndex: -1
} );

Or maybe:

this.tabs.not( this.active )
    .attr( {
        "aria-selected": "false",
        "aria-expanded": "false",
        tabIndex: -1
    } );

But I didn't expect it to be:

this.tabs
    .not( this.active )
        .attr( {
            "aria-selected": "false",
            "aria-expanded": "false",
            tabIndex: -1
        } );

Though I understand that's the rule as written.

---

Dave commented: The first looks best to me as a reader and is still very clear. I'm not sure how the style guide should balance human readability vs absolute rules that can be enforced by a formatter, but it seems sad to lose the former in search of the latter.

---

I added: My second commit was created by hand, no esformatter involved. Both esformatter and my esformatter-jquery-chain plugin tolerate line breaks as-is. The plugin only fixes indent.

[dmethvin]

I think it definitely makes sense to line-break, indent, and match stacking operations when you are going to .end() them further down the chain. Extending that rule to cases like these seems excessive to me.

[dmethvin]

I can see how the rules get us here, but ... ick. I'd prefer something like

this.tabs.filter( function() {
  return $( this ).attr( "tabIndex" ) === 0;
} ).attr( "tabIndex", -1 );

[scottgonzalez]

This is effectively the same case as what I mentioned earlier. Since the first operation is a context change, we should be able to keep it on one line, which gets us to:

this.tabs.filter( function() {
    return $( this ).attr( "tabIndex" ) === 0;
} )
    .attr( "tabIndex", -1 );

[scottgonzalez]

Or does it end up being:

this.tabs.filter( function() {
        return $( this ).attr( "tabIndex" ) === 0;
    } )
    .attr( "tabIndex", -1 );

[dmethvin]

I'd prefer the second. Here's the way I'd look at it: With jQuery chaining we have two types of patterns.

1. Sequential code, which is represented here. Results may be filtered but only those filtered results are used later in the chain. Indenting doesn't do anything for us there, and is distracting.

2. Stacking/unstacking code, which occurs when there is some filtering operation and .end() is used further down the chain. Indenting is important there to mark the stack/unstack points.

[scottgonzalez]

So what's the verdict on this?

[jzaefferer]

By itself, this is good, right?

var x = this.tabs.filter( function() {
    return $( this ).attr( "tabIndex" ) === 0;
} );

So adding another call shouldn't change those lines except for removing the semicolon:

var x = this.tabs.filter( function() {
    return $( this ).attr( "tabIndex" ) === 0;
} )
    .attr( "tabIndex", -1 );

Also having to indent the function body and closing braces makes this rule worse, in that it forces even more changes on unrelated lines.

[jzaefferer]

Based on the above, I suggest closing this PR, leaving things as they are. We can make the rules for chained method calls a bit more lenient, something like "Context changing method calls at the start of the chain are acceptable on the first line.", along with an example, e.g. the filter/attr chain.

[scottgonzalez]

Sounds good to me.

Issue filed: jquery/contribute.jquery.org#107