Skip to content

Tabs: Improve styling, adding line breaks on multiple chains #1508

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Tabs: Improve styling, adding line breaks on multiple chains #1508

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 35 additions & 27 deletions ui/tabs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,16 +352,19 @@ return $.widget( "ui.tabs", {
this._setupEvents( this.options.event );
this._setupHeightStyle( this.options.heightStyle );

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

// Make sure one tab is in the tab order
if ( !this.active.length ) {
Expand Down Expand Up @@ -412,16 +415,18 @@ return $.widget( "ui.tabs", {
}
} );

this.tabs = this.tablist.find( "> li:has(a[href])" )
.attr( {
role: "tab",
tabIndex: -1
} );
this.tabs = this.tablist
.find( "> li:has(a[href])" )
.attr( {
role: "tab",
tabIndex: -1
} );
this._addClass( this.tabs, "ui-tab", "ui-state-default" );

this.anchors = this.tabs.map( function() {
return $( "a", this )[ 0 ];
} )
this.anchors = this.tabs
.map( function() {
return $( "a", this )[ 0 ];
} )
.attr( {
role: "presentation",
tabIndex: -1
Expand Down Expand Up @@ -664,24 +669,27 @@ return $.widget( "ui.tabs", {
"aria-selected": "false",
"aria-expanded": "false"
} );

// If we're switching tabs, remove the old tab from the tab order.
// If we're opening from collapsed state, remove the previous tab from the tab order.
// If we're collapsing, then keep the collapsing tab in the tab order.
if ( toShow.length && toHide.length ) {
eventData.oldTab.attr( "tabIndex", -1 );
} else if ( toShow.length ) {
this.tabs.filter( function() {
return $( this ).attr( "tabIndex" ) === 0;
} )
.attr( "tabIndex", -1 );
this.tabs
.filter( function() {
return $( this ).attr( "tabIndex" ) === 0;
} )
.attr( "tabIndex", -1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or does it end up being:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

toShow.attr( "aria-hidden", "false" );
eventData.newTab.attr( {
"aria-selected": "true",
"aria-expanded": "true",
tabIndex: 0
} );
eventData.newTab
.attr( {
"aria-selected": "true",
"aria-expanded": "true",
tabIndex: 0
} );
},

_activate: function( index ) {
Expand Down